Don’t. Don’t do Pull Requests. But if you have to, continue reading.
Have you ever been ambushed by a Pull Request? It’s happened a lot to me. There’s a Pull Request on some software that you and your team are responsible for, and before you start reading all the changes, you’ve no idea what it is about. You’ve no idea what the author’s intent of the change is. You don’t know why the changes are being made. You don’t know if the author wants you to do an in-depth review, or just wants a token approval.
There are different types of Pull Requests, and they’re not all the same. With a little more effort, you can help the reviewers by letting them know what you expect from them. By helping your reviewers, you increase your chances of getting a good review and perhaps even an approval.
When you create a Pull Request, please make these things clear to the reviewers.
Types of reviews
- Token review: I just want a stamp of approval, quickly.
- Optional review: We pair programmed this, so we don’t need another reviewer, but we also don’t want to hide anything. Feel free to give feedback.
- Open review: I actually want feedback on this solution. I’m not sure I solved this in the best way. Do we, as a team, want to go this way?
Summarize your PR
Write a brief summary of your PR. Describe what you’ve done and why. Don’t just make a reference to a ticket, or a backlog item. That doesn’t make it obvious if your Pull Request is intended to solve the entire ticket, or if it’s just one of several upcoming Pull Requests related to the same ticket.
Add context to the PR
Link to other related Pull Requests. Give the reviewers an easy way to find related changes. This applies to both already merged PRs and open PRs. If there’s an order in which PRs need to be merged and deployed, make it explicit.
Who has been working on this PR?
Assuming you’re using version control, it will already be transparent who’s made the actual commits. However, if the code was written while pair programming, it’s not obvious from the version control that there are two people behind each commit. Make it explicit. If reviewers know that the code has been pair-programmed, they can act differently when reviewing, and a token approval might be good enough.
PRs with safe but noisy changes
If you want to do a lot of simple but “noisy” changes such as renaming, moving, formatting, commenting, and similar safe changes, consider creating a separate PR for those changes instead of bundling it together with a bug fix or a new feature. Bundling such changes together in the same PR might hide the actual fix/feature and make it harder to review.
The downside of this is that it adds more friction, and these types of changes may not get done at all. An alternative to a separate PR is to make the simple changes in separate commits.
Advice to reviewers
Let the author know that you’ll be reviewing their PR, so that they don’t merge before you’re done.
Be clear about what sort of review you’ve done. Have you just given a token approval? Maybe you don’t really have time to do a proper review, and the PR has already been paired on and you trust the authors. Then state that.
Be gentle sometimes. Be sensitive to your colleagues having a bad day. Maybe they have had a hard time with another PR already and need a quick win. If you don’t have strong opinions and arguments why something needs to change, maybe just give them a quick approval and a “LGTM!” (Looks Good To Me)
If they’ve added a new test, add some quick praise in a comment: “Nice!” Especially if they’re applying the boy scout rule and improving their surroundings. A little encouragement can go a long way.
Be hard sometimes. Imagine that you get a PR with no tests, lots of bad duplication, and several obvious code smells. Imagine that the PR has been worked on for a whole week, and the author has been working on it solo without having proposed a solution to the team beforehand. Maybe a new questionable third party library with 3 stars on GitHub has been added to the dependencies. This PR is going to take some effort to transform into something that is acceptable to merge. This is a time when it is worthwhile to be hard and say: “No, this is not acceptable.” Approving it without genuinely agreeing with the solution will cause even more effort and grief in the near future.
Want to read more on this topic? See Advantages of Trunk-only development
