Pull Requests—The Good, the Bad and Really, Not That Ugly
Recently, I stumbled upon an article on arkency—Disadvantages of pull requests.
I soon realized this was interesting stuff, worth sharing with my colleagues. Immediately I started writing a follow-up article, debating every claim made in the text.
We practice PR review workflow in Productive’s engineering team daily. This workflow helps us with:
- Ensuring code quality and stability
- Domain knowledge sharing in the team
- Organized mentoring activities
Productive’s Engineering team builds one SaaS product and works on two massive codebases—the backend (API) and the frontend (web app).
Every process comes with overhead, and so does this one, but I’ll argue it’s worth it.
In this blog post, I’ll quote key points from the mentioned article and give my thoughts on the matter.
Now, let’s discuss the disadvantages of pull requests!
1. More Long-Living Branches, More Merge Conflicts
PRs promote developing code in branches, which increases the time and the amount of code staying in a divergent state, which increases chances of merge conflicts. And merge conflicts can be terrible, especially if the branch waited for a long time.
Yes, this is true. But it shouldn’t be that big of an issue if you practice a good distribution of work in your team.
Most products consist of many modules which are usually different screens or sections in the app. You should always divide work across those modules, in a way that one PR doesn’t interact with the other.
If your codebase is not that modularized, you can ensure a few good practices to avoid this issue:
- Always split a big chunk of work into smaller, but functional PRs.
- Rebase the branch daily and update it with new changes from the master/develop (I usually do this before every code review).
- Always separate preparatory refactorings and merge them before implementing the actual feature changes. This is great advice given in the original article!
The most common mistake developers do is putting too much work into one PR, making both the review and merge process harder than it should be.
2. The Reviewability of a Change Decreases With Size
PRs tend to promote reviewing bigger chunks of code.
More often than not, developers fall into this trap. They think something like “I’ll just refactor this while I’m already here” or “I need to complete the whole thing before sending it for review”. This leads to oversized PRs and a slow development process.
3 PRs with 100 lines are reviewed and merged faster than 1 PR of 300 lines.
Of course, not all changes are the same. If we have 100 lines of boilerplate code, which is usually framework scaffolding, there’s no complexity there. Tests take a lot of lines but should be easy to read. Code with fewer dependencies will always be easier to grasp too.
Basically, the more a developer is experienced and familiar with the codebase, the more lines he/she is allowed to put in one PR. Less experienced developers on the project should strive to make their PRs as thin as possible.
3. Short Feedback Loop Makes Programming Fun
You code something up but it’s nowhere close to being integrated and working. You now have to wait for the reviewer, go through his remarks, discuss them, change the code…
I couldn’t agree more. There’s nothing more exciting in programming than getting a great idea, implementing it fast, and testing it out instantly.
The original author’s claim is that PR reviews kill that short feedback loop which makes programming less fun. But if that’s true, then it also means that:
- Your team is producing large and overcomplicated pull requests.
- You don’t have a good deployment and review environment setup.
- You’re not coding features in safe-enough isolation.
Again, it all revolves around making small and easily reviewable PRs, but we’ve already acknowledged that, so let’s move on.
If you do have a large PR waiting review, that doesn’t mean you can’t try it out in the wild. You could easily deploy the code to a staging environment, or even production, but on a separate domain so only you and your team can try out the new changes.
At Productive, for our main frontend app, we have an automated PR deployment setup. When you create a new PR on Github or push commits to an existing PR, a Github action is triggered that deploys the code to the “review” environment.
It takes the name of the git branch and deploys the frontend app to the subdomain with the same name. For example https://cool-new-stuff.review.productive.io.
Voilà, now you can try your PR changes in the live environment two minutes after you’ve created the PR.
Plus, there’s another Github action that posts a comment with a test link, so you don’t need to type it into the browser’s URL bar manually:
This is a frontend app example hosted as a static asset on a CDN, but you could do it similarly with backend apps.
There’s another mechanism for achieving a short feedback loop which is safe if done properly. With feature flags, you can ensure that your PR is not introducing any breaking changes and that new code is implemented in isolation from existing features.
Feature flags allow you to try the new changes in an isolated scope, without changing anything in the rest of the system. The scope can be either one user, one account, or just one API request. It’s a powerful mechanism that our team uses on a daily basis. It allows us to push new changes to the live environment rapidly, without worrying about breaking things.
4. Reviews Tend To Be Superficial
Proper review takes the same amount of focus as actual coding. Why not pair-program instead?
I agree that PR reviews tend to be superficial, especially when there’s too much to review. That only means that the reviewer is doing their job poorly. You can recognize this situation when you see a complex PR that only has code-style comments like “Fix naming” or “Wrong indentation”, etc.
If the PR seems like it’s too complicated—why not pair-review instead?
Pair programming (not to be confused with pair-reviewing) is useful for mentoring junior developers, but it’s a waste of time in a lot of other cases. That’s because you’re not solving complex problems all the time.
A better way to do it is to pair-review the PR with the author after most of the boilerplate is done and proper research has been made. Then you only talk about the complex stuff and you can leave the author of the PR to explain their thought process in detail.
There’s one more thing here… Programmers usually get this “feeling” when something stinks about their solution. They should be honest and leave comments in the PR, in every place where they feel they could be doing something wrong. That greatly increases the reviewability of the PR.
5. Merging Is Blocked by Remarks That Shouldn’t Be Blocking
Remarks made by the reviewer can fall anywhere on the spectrum of whether they should block merging or not: from a mistake that’ll bring production down to cosmetic suggestions or opinions.
These situations happen quite often with inexperienced reviewers. When commenting on the PR, you should tell the author if the comment is a merge-blocker or not, and never insist on details too much.
When I’m working with inexperienced developers, I’ll usually tell them exactly how to fix something, with an explanation. For non-blocking mistakes, I’ll just comment to watch out for that in the next PR. Usually, I’m the one to merge the PR, so they don’t have to wonder whether the PR is finished or not.
When reviewing experienced dev’s PR, I’ll tell them what I think about the problematic places in the code, but I’ll mostly approve the changes and leave them to the author to do the fixes the way they want to.
Not everything the reviewer says is always correct or the right thing to do. The author should be the one to decide on the comments—because it’s their work.
6. It’s Easier To Fix Than To Explain the Fix
The original author has to understand it first, agree with it, and then is expected to implement it. Often it’s better to let the original author merge his thing, and let the reviewer implement his remark post-factum?
A lot of times it’s hard to explain the fix to the author and it would be more efficient just to implement the fix yourself. But merging the unfinished PR could be dangerous, so I’d never suggest that. You’d also be missing the opportunity to educate the original author about the problem.
Pull requests don’t have to be done by only one person. Why wouldn’t you, the reviewer, just pull the branch, implement, and push the fixes back upstream? That way the author gets notified about what you did and you both have an opportunity to discuss the issue.
That’s also an effective thing to do when there are a few simple fixes required on the PR. Let’s say you notice a typo somewhere in the PR. It’s much faster to fix it yourself than to leave a comment and wait for the author to do the fix. You could do that directly in Github and it would take like 30 seconds.
7. Developers Are Slower To Adapt the Responsibility Mindset
The second one knows that every line they write can screw up things for other developers or even bring production down. They watch their step, they know they are the only one responsible for this change. It shortens the delay between making a mistake and seeing the effect of it.
This is an interesting and important claim. Can your business afford to tear the live server down just to teach the developers some responsibility? Sometimes it’s worth it, but in most cases, you wouldn’t want to do that.
Code stability is not the only reason why we should be reviewing our PRs. There are also benefits of mentoring, knowledge sharing, better release organization, feature completeness, structured discussion around a diff, etc.
At Productive, we encourage people to self-merge PRs without a review when they’re confident that the changes are fine. The developer should have the freedom to do that in the part of the codebase where they have more ownership. I believe this is how the responsibility mindset can be trained. When people build something from the ground up, they will feel the responsibility to keep everything in great shape.
8. PRs Discourage Continuous Refactoring
It’s good to follow boy scouts’ rule: always leave the place better than it was before. With PRs, though, this rule is harder to apply.
PRs will slow you down on refactoring. When you realize that some refactoring is needed, you need to switch to another branch to implement the refactoring separately, submit the PR to review, wait for the approval and merge the changes back into the original branch.
But it’s still better to do it that way, in my opinion. Refactoring PRs shouldn’t be complicated to review since there shouldn’t be any changes in the logic, only the code structure. That’s especially true if you have good test coverage. If you don’t, then maybe you should write the tests first?
Maybe the refactoring PR doesn’t need to go through the review process if everything is trivial. If it does, then mention to the reviewer that this is a refactoring-only PR that’s blocking feature development and the reviewer should prioritize the approval.
Having good team communication around PRs will minimize most of the downsides of the PR review process.
9. Negative Emotions
Mandatory PR reviews can induce way more negative emotions than needed. We all have limited emotional budgets — it’s better not to waste it on avoidable stuff.
When it comes to pull request reviewing, the responsibility is both the author’s and reviewer’s. There’s one simple rule to follow:
Don’t be a prick when reviewing or authoring the PR.
The original article is basically saying that we should let developers do what they want because they might get offended by the review comments.
Obviously, that’s ridiculous. This doesn’t have anything to do with PR reviewing itself—it’s a communication problem. If your engineering team doesn’t know how to communicate, then you’ll have bigger issues than developers offended by a PR review.
10. How Do You Switch to Branches With Migrations
You obviously sometimes need migrations while working on a branch. What do you do if you then have to switch back to another branch locally? Cumbersome.
Well, just do the migration separately on the main branch, before implementing the remaining logic in the PR. The world won’t collapse if you don’t always work on branches with PR reviews. You can always omit the process if you have a good reason for it.
Conclusion
The original article contains a list of suggestions on how to improve your team’s PR workflow. Those are all great ideas, which I encourage you to try. Thanks to arkency for writing a great and educational article! We’ve also published a copy of this article on Medium.
As with other organizational processes, we shouldn’t take them too seriously because they tend to generate overhead and slow work down. We should be pragmatic about PR reviews and when we notice they’re becoming a burden—we can skip them sometimes.
Don’t follow the rules blindly, try thinking with your head and do the right thing depending on the situation you’re in.
And a little remark for the end—let’s all be humble and respectful towards each other while reviewing each other’s code!
