The Theatre of Pull Requests and Code Review
1–10 of 431 posts
Re: The Theatre of Pull Requests and Code Review
#2I mean, stacked PRs are a thing for a reason
Re: The Theatre of Pull Requests and Code Review
#3If you split all the changes for a feature this way not only you hide the way all changes interact with each other but also make the development at least 10x longer because an average approval time is often more than a day.
Re: The Theatre of Pull Requests and Code Review
#4I mean, stacked PRs are a thing for a reason
I can also recommend rather to use the stacked PR approach. We have it since years, PR review "issues" are not a thing for us.
I still encourage do to a lot of small commits with good commit messages, but don't submit more then 2-3 or 4 commits in a single PR...
Re: The Theatre of Pull Requests and Code Review
#5PR 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 interpretation" style PR just seems like it's doomed to become lower priority for busy folks. You can add a fancy narrative to the comments if you like, but the root problem is that presenting a change block as-is and asking for a boolean blanket approval of the whole thing is an expensive request.
There's no substitute for shared understanding of the context and intent, and that should come out-of-band of the code in question.
Re: The Theatre of Pull Requests and Code Review
#6There’s a fine difference between a) splitting a big feature in parts that interact with each other via well defined interfaces, and b) splitting a big feature in parts that are suitable for PRs
You can split a big feature in N MRs and that doesn’t necessarily mean the N MRs are easier to understand (and so to review) than a single big MR. Taking into account the skills of the average software engineer, I prefer to review a single big MR than N different and not very well connected MRs (the classic example is that MR number N looks good and innocent, but then MR number N+1 looks awful… but since MR number N was already approved and merged the incentives are not there to redo the work)
Re: The Theatre of Pull Requests and Code Review
#7PR 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.
Re: The Theatre of Pull Requests and Code Review
#8300 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 needs you can choose anything from no review at all to 3x time reviewing than coding. What is right depends on your situation. PR is a tool.
Re: The Theatre of Pull Requests and Code Review
#9If you split all the changes for a feature this way not only you hide the way all changes interact with each other but also make the development at least 10x longer because an average approval time is often more than a day.
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.
Re: The Theatre of Pull Requests and Code Review
#10Agree 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 have the minimal number of commits (just what dossn't make sense standalone). Granted this take experience generally and experience with your reviewers which is where metrics like LoC counts can come in handy.