I Hate Pull Requests

Hate is a strong word, but I do dislike pull requests, and I have good reasons for doing so. Here are eight reasons why.

A halt on the way to value

1. They’re there for unclear purposes

There are many possible reasons to have a mandatory code review step in your process. Examples:

• Spreading knowledge about code changes
• Finding defects and security holes
• Learning from each other
• Increasing code readability and maintainability
• Adhering regulatory standards

But few teams have made a conscious decision about what they want to achieve with their code reviews.

Not knowing why they do it means that no healthy discussion can take place on how to do it, what kind of comments to leave, what to look for, etc. Neither is it possible to discuss alternative methods that might serve the same goals.

2. They increase lead times

No surprises here. Assuming that you have a sound continuous delivery process in place, adding a manual step in it will of course add to time from idea to possible value-add for end users.

How much? That depends of course on the size of the change, how you prioritize code reviewing vs other work, and whether you do tick-off reviews or do it like you mean it.

3. They increase WIP

Once you’ve submitted one piece of code to review, you start working on something else, right? Because you can’t just sit and wait for someone to pick up your PR. So now you have two things in flight. If you’re unlucky the next task is touching the same code as the first one, so you’ll eventually have merge conflicts with yourself. Or they depend upon each other, so you’ll have to base your next feature branch upon the previous until it’s merged.

If reviewing PRs is not a prioritized task in your team, you might have time to pick up a third and fourth task as well, multiplying the complexity.

Once some reviewer has comments on your first task, you have to get back into that context to fix them or defend yourself or get it merged. That’s a context switch, and context switches are expensive for humans. Sometimes when the reviewers aren’t happy after the first round, you might have to switch back and forth a few times for each PR.

As if this wasn’t enough, I haven’t even mentioned that it goes both ways. So apart from the number of ongoing work items caused by your own work, you will have to add your own reviews of other people’s code. That’s quite some number of contexts to keep active simultaneously.

4. They come too late

Most people submit their code to review only when they are content with their work. Even in teams with a policy to submit draft PRs at an early stage, most people tend to keep their code to themselves at least until they stop feeling deeply ashamed of it.

So when the review process kicks in, quite a lot of time and effort has already gone into the solution. That means that the opportunity for changing the approach altogether has passed a long time ago. Which leaves the reviewer with the dreary options of

1. Making some comments on code style, local readability etc.
2. Causing a lot of trouble, wasted time and hurt feelings by proposing bigger changes or starting over

5. They’re too big or too small

If you’re like me, you want to make life easy for your reviewers. My notion of how to do that is to try and keep pull requests

• small
• … while still representing a meaningful change.

The problem is that those two properties are hard to combine. I consider a properly-sized pull request to contain about one hundred lines of code changes, maximum. Or something on that scale, depending on language, context etcetera.

Bigger changes will make it very hard for the reviewer to properly understand what has happened and the possible consequences of it. That means they’ll have to either

• just skim over the code, leave some style comments and miss all the bugs
• spend hours or days playing around with the code to understand it well enough to add a valuable review

OK, so big changes are bad. The problem is that small changes are usually bad for review too, because not all meaningful changes fit within a hundred lines of code. So to keep pull requests small, you have to chop up your scope into non-meaningful parts. Which again makes it hard for reviewers, because adding a meaningful review of a change without its full context in mind is probably impossible.

6. But generally, they’re too darn big

I have to admit though that I’ve never seen any pull requests with the ‘too small’ problem in real life. I believe that’s because there are some mechanisms that quite efficiently nudge pull requests into getting big and fat.

1. As I already mentioned, people are usually willing to submit only ‘finished’ and ‘complete’ solutions to review
2. The ceremony of creating and later merging a pull request incurs a cost, so people tend to throw in all kinds of stuff into one pull request to avoid the work of creating another one.
3. The above is especially true if ‘the other stuff’ is cleanup work that you’d like to do before you start implementing the actual change.

Big pull requests are bad, not only because they’re harder to review, but also because

• Big sets of changes that go into production at once means bigger risks of defects, downtime etc.
• Big sets of changes that go into production at once makes it harder to debug production issues and to roll back deploys
• Big sets of changes usually take a longer time to produce, meaning that the code stays out of the mainline longer. Code staying outside the mainline increases the risk of hairy merge conflicts, which is a source of defects.

7. They conflate concerns

If we take a look at the list of possible reasons for having mandatory code reviews at the start of this post, only two of them motivate doing the review prior to deploying the changes to production: ‘Finding defects and security holes’ and ‘Adhering regulatory standards’.

For all other purposes the code review might just as well be done after production deploy. That means that we conflate the concerns of

• making sure the code is ready for deploy

with other concerns like

• Spreading knowledge and increasing maintainability

Why is this bad? It’s bad because lead times from idea to end user increases when we take the non-vital-for-production-readiness concerns into account on the way to production.

Every minute spent in a PR debating over which structure would be most readable for a future maintainer, or which phrasing in the README is more clear, is a minute lost for getting feedback on the actual value of the feature we’re implementing.

8. They foster fear and conflicts

This is my last point, but also my most important one.

When I discuss possible solutions with my colleagues at a white-boarding session, I feel totally okay when someone has another idea on how to solve the problem. We can discuss pros and cons and then decide on what to try out first.

When someone shows me how to do a nice coding trick or propose another way of structuring the code we’re writing in a pair or mob session, I take no offence at all.

But when the same kind of proposals turn up in written form in pull request comments, I do take offence. I do get annoyed. Then I take a few breaths and realize that no harm was meant. These comments are there because the person who wrote them think that they will help us to a better, more maintainable code base. But I have to consciously manage my hurt feelings in order to handle the comments in a productive way. They feel like a slap in the face, every time. And I believe I’m not alone.

There are many reasons to feel hurt or get annoyed at pull request comments:

1. They come at a stage when you already felt finished about the thing. You’ve already closed your mind to changes.
2. They come in written form. Written form makes everything sound less friendly.
3. When you don’t agree with the comments, explaining why in written form as non-aggressive as possible takes a lot of effort and might lead to an infected conflict.
4. A lot of reviewers forget to mention the stuff they like. Getting a lot of negative comments on your work without balancing it with positive feedback makes you feel useless, unless you have very thick skin.

I believe that the fear of negative emotions and risk of conflicts is a main reason why people work far too long on perfecting their code before they submit it to review.

I believe that the mechanisms of pull requests makes competent people feel untrusted, powerless and fearful.

I believe that working effectively with pull requests requires a thick skin or a big reserve of courage. Courage is a limited resource and we need it for a lot of things in our daily lives. If there is one place where we could spare it, we should.

Epilogue

Congratulations! You’ve reached the end of my rant. I hope it was worth the effort. If you’re still using pull requests when you’ve read this far, I hope it’s because you’re in one of the situations where they actually make sense:

1. When you work on an open source project with lots of contributors you know nothing about
2. As a means to propose changes to another team’s code base
3. When you work in a team that’s distributed over time zones (which is maybe a situation you should get out of anyway?)

However, whatever reasons you have for using pull requests, here are some nice resources on how to do them properly.

• A simple set of rules for clearer intentions with your review comments: https://www.selfelected.com/code-reviewing-comments/
• An excellent podcast on code review in general: https://www.se-radio.net/2020/02/episode-400-michaela-greiler-on-code-reviews/
• How to do code reviews like you mean it: https://chelseatroy.com/2019/12/18/reviewing-pull-requests/

Thanks for reading! I consider this post a draft PR, a place to gather my thoughts on this matter. So please let me know what I’m wrong about!