Readit News logoReadit News
computronus commented on The Theatre of Pull Requests and Code Review   meks.quest/blogs/the-thea... · Posted by u/todsacerdoti
davedx · 3 months ago
It's a very common refrain but I don't really agree with it:

"How do you create a PR that can be reviewed in 5-10 minutes? By reducing the scope. A full feature should often be multiple PRs. A good rule of thumb is 300 lines of code changes - once you get above 500 lines, you're entering unreviewable territory."

The problem with doing this is if you're building something a lot bigger and more complex than 500 lines of code, splitting that up across multiple PR's will result in:

- A big queue of PR's for reviewers to review

- The of the feature is split across multiple change sets, increasing cognitive load (coherence is lost)

- You end up doing work on branches of branches, and end up either having to become a rebase ninja or having tons of conflicts as each PR gets merged underneath you

The right answer for the size of a PR is NOT in lines of code. Exercise judgement as to what is logically easier to review. Sometimes bigger is actually better, it depends. Learn from experience, communicate with each other, try to be kind when reviewing and don't block things up unnecessarily.

computronus · 3 months ago
I do agree with the common refrain, actually, and disagree with the idea that work can be so big and complex that it has to be in one pull request.

> A big queue of PR's for reviewers to review

Yes, yes please. When each one is small and understandable, reviewers better understand the changes, so quality goes up. Also, when priorities change and the team has to work on something else, they can stop in the middle, and at least some of the benefits from the changes have been merged.

The PR train doesn't need to be dumped out in one go. It can come one at a time, each one with context around why it's there and where it fits into the grander plan.

> The [totality] of the feature is split across multiple change sets, increasing cognitive load (coherence is lost)

A primary goal of code review is to build up the mental map of the feature in the reviewers' brains. I argue it's better for that to be constructed over time, piece by piece. The immediate cognitive load for each pull request is lower, and over time, the brain makes the connections to understand the bigger picture.

They'll rarely achieve the same understanding of the feature that you have, you who created and built it. This is whether they get the whole shebang at once or piecemeal. That's OK, though. Review is about reducing risk, not eliminating it.

> You end up doing work on branches of branches, and end up either having to become a rebase ninja or having tons of conflicts as each PR gets merged underneath you

I've learned not to charge too far ahead with feature work, because it does get harder to manage the farther you venture from the trunk. You will get conflicts. Saving up all the changes into one big hunk doesn't fix that.

A big benefit of trunk-based development, though, is that you're frequently merging back into the mainline, so all these problems shrink down. The way to do that is with lots of small changes.

One last thing: It is definitely more work, for you as the author, to split up a large set of changes into reviewable pieces. It is absolutely worth it, though. You get better quality reviews; you buy the ability to deprioritize at any time and come back later; most importantly for me, you grasp more about what you made during the effort. If you struggle to break up a big set of changes into pieces that others can understand, there's a good chance it has deeper problems, and you'll want to work those out before presenting them to your coworkers.

computronus commented on Setuptools version 78.0.1 breaks install of many packages   github.com/pypa/setuptool... · Posted by u/computronus
computronus · 9 months ago
The original report is just for ansible-vault, but comments indicate widespread effects, hence the altered title here.
computronus commented on The ideal candidate will be punched in the stomach   scottsmitelli.com/article... · Posted by u/smitelli
computronus · 10 months ago
Reminds me of a job posting sent by a recruiter that expected candidates to seek "professional and personal hypergrowth", "keep up with an unrelenting pace", and "thrive on change". Dealing with these facets of work in moderation is all well and good. However, these and other points led me to guess that they had set up a high-pressure, possibly chaotic environment, perhaps on purpose.

I opted not to pursue the opportunity.

computronus commented on Dropbox announces 20% global workforce reduction   blog.dropbox.com/topics/c... · Posted by u/mfiguiere
UniverseHacker · a year ago
One hardly needs $2B to slack off on a boat in the Caribbean… many people do so on surprisingly nice ~30 foot sailboats you can buy with a few months work at minimum wage, and wild seafood is free

I see a lot of people that say “If I won the lottery I’d….” and then describe something they could definitely do without winning the lottery… which makes me think they still would not actually do so if they won

computronus · a year ago
Lawrence: Well, what about you now, what would you do [if you had a million dollars]?

Peter: ... Nothing.

Lawrence: Nothing, huh?

Peter: I would relax, I would sit on my ass all day ... I would do nothing.

Lawrence: Well you don’t need a million dollars to do nothing, man.

https://www.youtube.com/watch?v=4lmW2tZP2kU

computronus commented on Product management is hosting a party, not playing chess   tidyfirst.substack.com/p/... · Posted by u/KentBeck
klodolph · a year ago
Seems like there are some steps missing between the quoted at the beginning and the outrage in the article. I don’t really understand the specifics of the complaint.

My main thought reading the quote at the top is, “you shouldn’t call people ‘neckbeards’”. But it is also true that not all engineers want to talk to customers. Don’t judge a fish by how well they ride a bicycle, and all that. “It takes all kinds” is a beautiful expression to sum that up.

computronus · a year ago
By my reading, the outrage is likely coming from the cushioned stereotyping from the podcast participant. They're stating that "neckbeard types" and autists aren't customer-ready and are uncomfortable talking with customers. That second part is worse because it may lead to assuming that those folks would never want to talk to customers, or even learn how, and so the PMs might "protect" them by never offering them the chance, while assuring themselves "that's okay, that's great, I mean it takes all kinds".
computronus commented on Spotify will reduce total headcount by approximately 17%   newsroom.spotify.com/2023... · Posted by u/filleokus
CamperBob2 · 2 years ago
So many of the recent layoffs have not accounted for individual performance or criticality to the business

And you think a union will?

Unions make more sense when the workers actually are interchangeable. Are you?

computronus · 2 years ago
It doesn't matter, if my employer would treat me as interchangeable anyway.

And even interchangeable employees deserve reasonable accommodations. I do think a union can highlight those needs more effectively than individuals (especially for interchangeable ones, to your point).

u/computronus

KarmaCake day274July 10, 2015View Original