I Don't Like Pull Requests

23 Aug 2024

How about this for a code review process: one person goes away, does some amount of work resulting in code changes, and then creates a PR that others review more-or-less at their convenience. Ideally, they work in small increments so the review can be more focused. Feedback is shared through threads of comments, and once there is some level of approval, the changes can be merged into the mainline of the repository. Rinse, wash, repeat.

People have told me that this style of code review improves the quality of code, reduces defects, and helps share knowledge amongst the team. I’ve yet to see any of this happen, and I have tried to honor the promise of what they can provide. What I have seen are teams with very little communication outside of sparse, scattered daily rituals, where the petty misunderstandings fomented by asynchronous text communications become the only point of contact between “teammates”, resulting in nothing but friction and quiet antipathy.

Code is an artifact. It is not a crystalline record of someone’s intentions. It is formed in the context of an understanding of a problem, pressures to “get things done”, influences of existing code, considerations of the future—it did not just fall out of a coconut tree. Likewise, it’s very difficult to glean this context from looking at the static “product” at the point where someone raises a PR. Consider how deeply complex any aspect of written code can be—I have a book on the naming of variables!

Meaningful feedback at this point will require a rewinding of the process. In a sense, you might be able to see “what” someone did, but you are really going to have to work to understand why they did it.

I’ve tried many different approaches to how I structure my own PRs, and how I interact with others’. I’ve left (what I thought to be) very thoughtful comments on my own and others’ work, only for them to be ignored with no interaction. I’ve sought out more detail from others when they leave comments, which more often then not doesn’t really go anywhere because they have have their own work to be concerned with and don’t have the bandwidth necessary to have the conversation.

Dragan Stepanović has done great work illustrating how asynchronous code reviews create a trade-off between code quality and speed. This is with the generous assumption that those code reviews actually improve code quality to begin with. I strongly agree with his idea that the ideal PR is “1 line of code, reviewed as it’s written”. There are ways to do that, but you’ll have to learn to actually work together.

Here’s what I think is the best approach to reviewing a PR asynchronously: approve it. This doesn’t mean you can’t leave comments, but don’t hold your coworker hostage—consider doing non-blocking reviews. And be honest: what are you going to catch? A defect? Formatting? A typo? Those things are much better solved with automation and better testing practices—you might say we can’t solve those with code reviews. Are you going to suggest an architectural change? That’s certainly more interesting, but you’re really going to send someone back to the drawing board? If your “team” is going to be working on things alone, let’s actually trust them to do that.

Further reading from people smarter than I