Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

The Theatre of Pull Requests and Code Review

meks.quest

Post reply on HN
Print view View original

41–50 of 431 posts

Re: The Theatre of Pull Requests and Code Review

#41

Post by davedx »

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.

Re: The Theatre of Pull Requests and Code Review

#42

Post by whstl »

smashedtoatoms wrote: post #28
Supermancho wrote: post #23

> Story-Telling Commit Messages No thank you. Talking to future ME, I don't need to know how I got to what I want me to look at. A squashed ticket-by-ticket set of merges is enough for me.

I'm editing this to be nicer. I'm really trying to be nicer. Consider the possibility you're not the only one in the codebase and that the git history might provide the why to the code's what.

As someone who dives in the commit story often:

If it's a pristine, linear, commit story, sure.

If it includes merges commits, "fix" commits, commits that do more than one thing, detours, side-quests, unrelated refactors then squashing is 100x better.

Re: The Theatre of Pull Requests and Code Review

#43

Post by qiine »

enlyth wrote: post #18
nenenejej wrote: post #8

300 LOC in 10 minutes. Or 2 sec per loc. Or for average 30 char line, 600wpm reading speed. OK. There is little you can review properly in 10 minutes unless you were already pairing on it. You might have time to look for really bad production-breaking red flags maybe. Remember the underlying reasons for PR. Balance between get shit done and operational, quality and tech debt concerns. Depending on what your team need…

I like to actually checkout the branch I'm reviewing and run the code myself to observe if it does what is claimed, that usually takes up at least 10 minutes in itself, sometimes more. From my experience most of the issues I find are actually from this type of observation rather than actually reading the code and imagining what it does in my head.

the mind remains a poor compiler ;p

Re: The Theatre of Pull Requests and Code Review

#44

Post by xmcqdpt2 »

jvanderbot wrote: post #5

PR review is probably at least a little performative. But I trust my colleagues to do good reviews when I ask them to, and to ignore my PRs when I don't. That's kind of the way we all want it. I regularly ask for a review of specific changes by tagging them in a comment on the lines in question, with a description of the implications and a direct question that they can answer. This, "throw the code at the wall for in…

This is a huge pet peeve of mine. At work I'm an expert on part of the code base that sees a fair number of contributions.I get many private IMs from colleagues asking me to "Approve please" or something like that with a dump of 100s of lines, of which maybe 10 lines are relevant to me (touch files or behaviour that I'm an expert on, hence why they need my approval.)

Minimally, I would like context for the change, why it required a change to this part of the codebase, and the thought process behind the change. Sometimes but not often enough I send the review back and ask them for this info.

IMO many software developers are just not fast enough at writing or language so providing an explanation for their changes is a lot of work for them. Or they are checked out and they just followed the AI or IDE until things worked, so they don't have explanations to provide. People not taking the time is what makes reviews performative.

Re: The Theatre of Pull Requests and Code Review

#45

Post by osigurdson »

The PR type approach comes from the Linux kernel where there is essentially a hierarchy of gatekeepers with increasing trust and responsibility. It is a very individualistic type approach (i.e. if you merged it into your branch, it's on you and speed is a non-goal for the most part). This is often different from many software projects as it is like a collective where often, no one really has individual responsibility for anything (it is more like collective responsibility), speed is everything and there are certainly no gate keepers - maybe the review actually doesn't matter that much in this context.

Re: The Theatre of Pull Requests and Code Review

#46

Post by mtlynch »

I love code reviews and blog posts about them, but I vehemently disagree with all of this advice.

>His example PR[0] adds just 152 lines of code, removes 2 lines, but uses 13 thoughtful commits.

>While some developers might understand those 152 lines from the final diff alone, I couldn't confidently approve it without the commit story.

This is ridiculous!

You absolutely can and should review a PR without demanding its "commit story."

Go read the PR under discussion here.[0] There's nothing about it that's hard to understand or that demands you go read the 13 intermediate steps the developer took to get there.

The unit of change in a code review is the PR itself, not the intermediate commits. Intermediate commits are for the author's benefit, not the reviewer's. If the author rewrote the code in FORTRAN to help them understand the problem, then converted it back to the codebase's language, that's 100% okay and is not something the reviewer needs to care about.

The PR should squash the individual PRs at merge time. The linked PR[0] is a perfect example, as the relevant change in the permanent commit history should be "Measure average scheduler utilization" and not "Collect samples" or "Mock sampling."

When you need to communicate extra context outside of the code, that should go in the PR description.[1] Your reviewer shouldn't have to go scour dozens of separate commit messages to understand your change.

>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.

5-10 minute reviews are so low that it's basically thoughtless rubber-stamping.

If someone spent 5-10 hours making a change, the reviewer should definitely think about it for more than 5 minutes. If all the reviewer is doing is spot checking for obvious bugs, it's a waste of a code review. The reviewer should be looking for opportunities to make the code simpler, clearer, or more maintainable. 5-10 minutes is barely enough time to even understand the change. It's not enough time to think deeply about ways to improve it.

[0] https://github.com/sasa1977/hamlet/pull/3

[1] https://refactoringenglish.com/chapters/commit-messages/

Re: The Theatre of Pull Requests and Code Review

#47

Post by osigurdson »

stillworks wrote: post #34
epage wrote: post #10

Agree with the overall sentiment but disagree with > A good rule of thumb is 300 lines of code changes - once you get above 500 lines, you're entering unreviewable territory. I've found LoC doesn't matter when you split up commits like they suggest. What does matter is how controversial a change is. A PR should ideally have one part at most that generates a lot of discussion. The PR that does this should ideally also…

In some parts of the industry number of CRs and revisions-per-cr is tracked as a performance metric. Many people learn to game this to make their "numbers" appear good i.e. high number of CRs and low revisions per CR.

Easy and fun to put metrics around random things. I'll start to squirm if you ask me to draw the connection between these metrics and NPV.

Re: The Theatre of Pull Requests and Code Review

#48

Post by Supermancho »

smashedtoatoms wrote: post #28
Supermancho wrote: post #23

> Story-Telling Commit Messages No thank you. Talking to future ME, I don't need to know how I got to what I want me to look at. A squashed ticket-by-ticket set of merges is enough for me.

I'm editing this to be nicer. I'm really trying to be nicer. Consider the possibility you're not the only one in the codebase and that the git history might provide the why to the code's what.

> Consider the possibility you're not the only one in the codebase and that the git history might provide the why to the code's what.

I can't win with HN critics. If I talk about someone else looking, then I'm assuming. If I talk about myself, then I'm being too self-centered (in the oblique sense you reference). I am very aware of how this works across teams of people, not just myself, since I'm in industry.

Re: The Theatre of Pull Requests and Code Review

#49

Post by flir »

pards wrote: post #7
jvanderbot wrote: post #5

PR review is probably at least a little performative. But I trust my colleagues to do good reviews when I ask them to, and to ignore my PRs when I don't. That's kind of the way we all want it. I regularly ask for a review of specific changes by tagging them in a comment on the lines in question, with a description of the implications and a direct question that they can answer. This, "throw the code at the wall for in…

> and to ignore my PRs when I don't PRs should be optional, IMHO. Not all changes require peer review, and if we trust our colleagues then we should allow them to merge their branch without wasting time with performative PRs.

I always appreciate an extra pair of eyeballs, even on a one-liner. Everyone's an idiot sometimes.

Re: The Theatre of Pull Requests and Code Review

#50

Post by fidotron »

StilesCrisis wrote: post #36
fidotron wrote: post #9

Earlier quoted context omitted.

This will be accompanied by the sort of dev manager that thinks a KPI for "number of PRs merged" won't in any way be gamed or backfire. I don't know what they're doing where you can do code reviews in 5-10 minutes, but in my decades doing this that only works for absolutely trivial changes.

The goal here is to make almost all CLs trivial enough that they can be reviewed quickly. You can compose almost any feature out of many small simple changes.

> You can compose almost any feature out of many small simple changes.

You can?

Post reply on HN