Pull Request review etiquette
As software engineers, we spend most of our time writing and reading code. A lot of that time is spent reading other people’s code in pull requests (PRs). They are probably one of the most important tools a team can have not only to ship good quality code but also to nurture a great relationship between all team members when done right.
Who?
Everyone on the team should review PRs, from the most recent joiner independently of the experience level to the most senior members of the team. We can have code owners set up for specific parts of the code that we want to keep to a certain standard, but with time anyone in the team exposed to that part of the code should easily be added to the same code owners group.
Having new starters reviewing PRs from the get-go is a great way to have them engaged and feeling a sense of ownership from day 1. For more junior developers, it is also an excellent learning tool to see other colleagues code and participate in discussions around it. And to conclude, as senior as someone might be they will always learn something with the process, more often than not from a junior member of the team.
When?
All the time. Reviewing PRs should be a daily task for everyone on the team. Slow PR turnover might not affect a small team a lot, but with the team growing, it can become a real problem.
With the size of a project or a team increasing, if people don’t review PRs often this result in a large number of stale PRs. The longer a PR stays open, the bigger the odds of having merge conflicts to solve. At the same time, context switching is probably one of the biggest productivity killers, and having people constantly jumping between different pieces of work because they are waiting on their PRs to get reviewed is not good.
Ultimately, every team wants to ship features as fast as possible and having a fast PR turnover time is one crucial part of the process.
A good tool we use at Babylon Health is Pull Reminders that gives us Slack reminders and notifications for our PRs comments or any PRs waiting for our review. My colleague Sakis Kaliakoudas wrote about our setup with Pull Reminders and the other Pull Panda tools here.
What?
Everything. Anyone should review any parts of the code. In a larger team spread in a cross-platform (we call them squads at Babylon Health), it’s easy to stop reviewing code that is not part of your squad. This eventually leads to information and knowledge silos that are not beneficial for anyone on the team. Initially, it might not be apparent how much this can impact the team, but soon if people go on holiday and someone else needs to jump in to solve any issue they’ll lack not only the domain knowledge for that part of the product, but they’ll also struggle to navigate a code base they haven’t seen evolving.
While people on the same squad will have more context on the features and will have a more complete overview when reviewing the code, someone from outside the squad will look at the code differently and can always offer a different perspective, for that reason we not only encourage people to review any PR, we actually use Pull Assigners (our setup is explained on the same article shared above) to have PR reviewers randomly assigned as well. We also require 2 reviews to merge any PR, so it’s easy to have someone from the squad with more context in the feature and someone from outside.
How?
We already spoke about everyone being encouraged to participate in PR reviews, that wouldn’t be possible with a nice PR review etiquette shared and followed by everyone on the team. We are true believers in psychological safety, so having this is essential. We want everyone to feel comfortable to comment any piece of code from whoever it is. A few etiquette rules we try to follow:
Suggesting vs Requesting
We always try to suggest changes rather than requesting. Bonus point if examples and some context for the suggestion are given.
Scope of the review
It’s important to keep the review under the scope of the PR. It would be nice for people to go and refactor or fix everything they touch, but that is not always possible so shouldn’t be something requested when reviewing PRs.
People in the team should aim to follow the boy scout rule, but demanding those big refactorings when they don’t make sense is not a good idea. Adding some comments about a potential refactoring just to trigger a conversation, or suggesting small changes should be fine most of the times, but it’s important to try and keep the review under the scope of the PR.
Nitpicks
It’s fine to have a bit of OCD, as developers we all do :) but it’s important not to block PRs because of nitpicks. Hopefully, most nitpicks would have been picked up earlier by all the automated checks on CI (e.g. static analysis). When they’re not, people often have follow-up work where they can take up the suggestions, no need to block PRs on those.
Praise
We all tend to look at PRs and always try to find something that requires a change. It’s good practice to also look at PRs with a different set of eyes and try to find things we can praise our colleagues on, rather than just change requests or suggestions. This will improve the quality of the relationships within the team and create an overall better environment for everyone.
Communicate
If a discussion on a particular thing ends up being more complex and not easy to address on the PR, author and reviewer should just have a chat and a pairing session if required. Having long asynchronous discussions on PRs is not the most optimal solution.
Conclusion
To wrap it up, a nice set of rules and etiquette around the team PRs together with the Pull Panda tools can do wonders for the team productivity. If you have any specific guidelines or tools you find useful, please share as we would love to know more about it. Happy PR reviews :)
Does this sound interesting?
If you want to help build the future of healthcare, you can check our open roles here.
