The Theatre of Pull Requests and Code Review

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

I see stacked PRs as independent of this. PRs are a good unit of cohesion of changes, particularly changes that only make sense if later changes are also merged.

stacked PRs are wildly reviewer-hostile, please do not do them

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.

> only you hide the way all changes interact with each

Splitting the change does not prevent you from looking at diffs of any combination of those commits. (Including whole pr) You're not losing anything.

> at least 10x longer because an average approval time is often more than a day.

Why do you think it would take longer to review? Got any evidence?

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

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.

For any PR above a few line change, if a developer has not done a self review, I don’t review it all.

Instead I request that it is self reviewed with context added, prior to requesting re-review.

I also tend to ask the question, “are any of these insights worth adding as comments directly to the code?”

9/10 the context they wrote down should be well thought out comments in the code itself. People are incredibly lazy sometimes, even unintentionally so. We need better lint tools to help the monkey brain perform better. I wish git platforms offered more UX friendly ways to force this kind of compliance. You can kind of fake it with CI/CD but that’s not good enough imo.

Exactly, if you want people to think about your code/changes you should be able to give them the needed context.

If you don't know them, please realize your code isn't automatically a gift everybody waited for, you may see it that way, but from the other side this isn't clear until someone put in the work to figure out what you did.

In short: added code produces work. So the least you should do is try reducing that work by making it easy to figure out what your code is and isn't.

Sum up what changes you made (functionally), why you made them, which choices you made (if any) and why and what the state of the PR code is in your own opinion. Maybe a reasoning why it is needed, what future maintenance may look like (ideally low). In essence, ask yourself what you'd like to know if someone appeared at the door and gave you a thumb drive with a patch for your project and add that knowledge.

Also consider to add a draft PR for bigger features early on. This way you can avoid programming things that nobody wanted, or someone else was already working on. You also give maintainers a way to steer the direction and/or decline before you put in the work.

>that should come out-of-band of the code in question.

Ideally, yes. After a decade and something' under ZIRP, seems a lot of workers never had incentive to remain conscious of their intents in context long enough to conduct productive discourse about them. Half of the people I've worked with would feel slighted not by the bitterness the previous sentence, but by its length.

There's an impedance mismatch between what actually works, and what we're expected to expect to work. That's another immediate observation which people to painfully syntaxerror much more frequently than it causes them to actually clarify context and intent.

In that case the codebase remains the canonical representation of the context and intent of all contributors, even when they're not at their best, and honestly what's so bad about that? Maybe communicating them in-band instead of out-of-band might be a degraded experience. But when out-of-band can't be established, what else is there to do?

I'd be happy to see tools that facilitate this sort of communication through code. GitHub for example is in perfect position to do something like that and they don't. Git + PRs + Projects implement the exact opposite information flow, the one whose failure modes people these days do whole conference talks about.

Agreed, 300 lines will take me a lot more than 10 minutes to review properly!

Depends on the specific changes of course, but generally speaking.

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.

Your linter/tests are for catching real errors. Review is to understand the shape of it mostly IMO. I could probably fairly easily review 300 loc if its not a particularly confused shape.

Good luck getting 90% of devs to commit (har har) to this level of history surgery. Of the ones that actually know how to do it (a small fraction of your typical engineer) an even smaller fraction of that is going to have the patience to do it correctly. You’ll tell devs to do this kind of thing and you’ll either have their eyes glaze over from lack of understanding, annoyance at the extra work, or nodding then apathetical disregard. The one top engineer will do it then be frustrated that the rest of the org doesn’t do proper commit hygiene.

It’s not really something you can easily enforce with automation, so basically unachievable unless you are like Netflix and only hiring top performers. And you aren’t like Netflix.

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.

It’s often an inverse correlation in well functioning environments. Controversial changes and bug fixes are small and targeted and are deeply reviewed for unintended side effects, while large change sets are often new work that’s been discussed upfront and follows well established patterns and gets waved through.

I think the other thing that often muddies the waters in discussions of code review is that open source projects and internal codebases are generally in rather different situations. An internal codebase is usually worked on by a fairly small group of experienced people, who are both creating and also reviewing PRs for it. So:

- the baseline "can I assume this person knows what they're doing?" level is higher

- making the "create PR" process take longer in order to make the review process faster is only a tradeoff of the time within the team

- if something is wrong with the committed code, the person who wrote it is going to be around to fix it

But in open source projects, there are much more often contributions by people outside the "core" long-term development team, where:

- you can't make the same assumptions that the contributor is familiar with the codebase, so you need to give things extra scrutiny

- there are often many fewer people doing the code review than there are submitting changes, so a process that requires more effort from the submitter in order to make the reviewer's job easier makes sense

- if there's a problem with the code, there's no guarantee that the submitter will be available or interested in fixing it once it's got upstream, so it's more important to catch subtle problems up front

and these tend to mean that the code-review process is tilted more towards "make it easy for reviewers, even if that requires more work from the submitter".

It sounds like a good job where the most important part is finding other people’s mistakes.

Though I do appreciate the shoutout to adding tests in CR. But returning a PR solely because it doesn’t have tests, is effective, but a little performative too. It kind of like publicly executing someone, theirs gotta be some performance for it to be a deterrent. If something doesn’t have tests my review is going to be a very short performance where I pretend read the rest of the code. Then immediately send it back.

A lot of developers are working in hellish companies, micromanaged and beaten down into cogs.

One very valuable skill for those of us who have experienced productive collaboration is learning how to introduce it to new places.

Sometimes that means telling the executives that their promotion process is making them less successful. Sometimes it means wandering around PRs leaving useful positive comments to seed the idea that PRs can be useful. Sometimes it means pointing out tests only when there is a bug, so people can experience how great it is to follow the practices that keep us from introducing bugs in the first place.

I wish that more CS programs would explicitly teach their students critique skills, the way art and music and english and math programs do. But until then, we're counting on engineers getting lucky and landing in a functional workplace like yours.

It's not "like" another task, it IS another task!

So don't do it in parallel.

Completely park other tasks, spend time on the review and record that time appropriately.

There's nothing wrong with saying you spent the previous day doing a large review. It's a part of the job, it is "what you're working on".

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.

Changing cowboy code is much, much slower than changing good code.

If you need to move fast for the next two weeks, sure. If you need to move fast for the next year, you are better off collaborating.

Your comment includes capital letters and punctuation! These are skills we all learn to ensure our writing is as legible as possible to the reader. Quotes, periods, commas, clauses, paragraphs etc. are all stylistic decisions that support the underlying text and message.

I contend that learning the art of story telling through a stack of patches is just as important and, once learned, comes just as naturally as utilizing vocabulary, grammar, syntax and style with the written word.

Allen Iverson got criticized by the media for “letting down his teammates” for skipping a few practices. He famously said “We’re talking about practice, practice! not the game! How is MY going to practice gonna make THEM better?!” He got flack for those comments, but what he said was accurate. If we’re talking about outcomes you’re beholden to the person who is seen as the difference maker, all the teamwork in the world isn’t going to improve an individual’s the lack of ability.

That is a performance issue, for sure, and those developers are throwing away some of the best feedback they can get.

A decent programmer can write code they can effectively work with. The really great programmers write code that even interns can effectively work with. The only way to get to that level is to get good at eliciting and taking in feedback from the people we want to be effective with our code.

It doesn't necessarily mean doing exactly what we are told: it means understanding the why of a comment, what underlying flaws or confusions a comment is pointing towards. It means encouraging people to ask questions in code reviews, rather than just leave commands: often a "how does this manage to do X?" comment points to a place where bugs were hiding anyway, even if also it is a chance to share a language feature.

Many engineers work in companies where being a really great programmer doesn't get you any points. Often the only reward for writing code that is easily modified later is the gratitude of a future developer asked to make a change to it years down the line.

But I am that developer often enough that that's still made the journey worth it for me.

That is 100% not my experience. People who do that tend to be perfectionists trying to cross all the ts.

Confused developers are just unable to create such reasoning.

You only know poor developers then, I guess

One of the advantages of small, frequent code reviews is that a team shares the broader context.

Which is far more valuable than ten minutes of extra typing.

> 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

+100 to this. My job should be thoughtfully building the solution, not playing around with git rebase for hours.

I agree with this. One way to keep changes small but still compose them into a coherent PR is to make each commit in the final PR independently meaningful, rather than what actually transpired during local development. TFA touches on this somewhat, contradicting the bit you quoted.

A trivial example would be adding the core logic and associated tests in the first commit, and all the remaining scaffolding and ceremony in subsequent commits. I find this technique especially useful when an otherwise additive change requires refactoring of existing code, since the things I expect will be reviewed in each and the expertise it takes are often very different.

I don't mind squashing the branch before merging after the PR has been approved. The individual commits are only meaningful in the context of the review, but the PR is the unit that I care about preserving in git history.

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

This is a feature. I would infinitely prefer 12 PRs that each take 5 minutes to review than 1 PR that takes an hour. Finding a few 5-15 minute chunks of time to make progress on the queue is much easier than finding an uninterrupted hour where it can be my primary focus.

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

It increases it a little bit, sure, but it also helps keep things focused. Reviewing, for example, a refactor plus a new feature enabled by that refactor in a single PR typically results in worse reviews of either part. And good tooling also helps. This style of code review needs PRs tied together in some way to keep track of the series. If I'm reading a PR and think "why are they doing it like this" I can always peek a couple PRs ahead and get an answer.

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

This is a tooling problem. Git and Github are especially bad in this regard. Something like Graphite, Jujutsu, Sapling, git-branchless, or any VCS that supports stacks makes this essentially a non-issue.

I also feel like what gets lost in this is not everything you are building is a bite size feature in large existing project. Sometimes you are adding an entire subsystem that is large to something relatively greenfield. if you broke that down into features, you will need 20 PRs and if you wait for review, or even don't wait but have to circle back to integrate lots of requested changes, what might be a couple of weeks of work turns into 2 to 3 months of work. That just does not work unless you are in a massive enterprise that is ok with moving like molasses. Do you wind up with something not as high quality? Probably. But that is just the trade-off with shipping faster.

Here's an alternative approach: Discuss the design with your team beforehand, and have active ongoing discussions, sanity checks, and even pair programming during the development process. That way the review is not an exhaustive end-to-end review with the reviewer coming in cold. It's instead the final approval step in a long chain of decisions that have already been discussed and agreed upon.

Of course that won't work for all projects/teams/organizations. But I've found that it works pretty well in the kinds of projects/teams/organizations I've personally been a part of and contributed to.

It's not about splitting up the PR: it is about splitting up the _work_.

If you don't have feature flags, that is step one. Even if you don't have a framework, you can use a Strategy or a configuration parameter to enable/disable the new feature, and still have automated testing with and without your changes.

> 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

This shouldn't matter unless you are squashing commits further back in the tree before the PR or other people are also merging to main.

If a lot of people are merging back to main so you're worried about those causing problems, you could create a long life branch off main, branch from that and do smaller PRs back to it as you go, and then merge the whole thing back to main when your done. That merge might 2k lines of code (or whatever) but it's been reviewed along the way.

I don't necessarily disagree with you. Just pointing out that there are ways to manage it.

A stack of PRs is much better for reviewers than a single massive PR.

Use jujutsu and then stacking branches is a breeze

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.

Keep merging each PR into master under a feature flag, that's how it's done. Huge PRs that implement a feature in one swoop are pretty much the worst case scenario for every stage: review, testing, deployment and monitoring.

100%. I think the right answer is to break features into atomic commits, but keep PRs at the feature level. This reduces the PR friction, while letting reviewers easily view change sets for specific features, and if a specific feature needs a patch you don't need to do any rebase gymnastics, just add a patch commit.

AI agents like frequent checkpoints because the git diff is like a form of working memory for a task, and it makes it easy to revert bad approaches. Agents can do this automatically so there isn't much of an excuse not to do it, but it does require some organization of work before prompting.

If security or bugs don't matter much for your project, sure

> Intermediate commits are for the author's benefit, not the reviewer's.

I don't even know how could commits only benefit the author; if they're poor they won't help him either, if not as a log of how much work he's done.

Unless you make a PR for every insignificant change, PRs will most often be composed of series of changes; the individual commits, if crafted carefully, will let you review every step of the work of the author quickly.

And if you don't eschew merges, with commits you can also group series of related modifications.

Hard agree.

Commits are not important. As an author, you should not waste your time on this. As a reviewer, just ignore them.

That's a bummer! At my company we've started investing in what I'm calling 'semantic linting', which is basically running GPT over a PR with a set of rules that we iterate on. Already I'm finding huge value for style/pattern comments that linters can't easily catch, dropping warnings for common DB migration footguns, or notifying people of changing patterns/new ways of doing things. Been great so far!

Don't worry, if you actually increase AI usage your team will be forced to automate code review, either explicitly or implicitly.