The Art of Pull Requests

As I wrote previously we have a very remote setup with team members spread all over the world. That means that code reviews and pull requests have to be done remotely.

Recently one of our team members proposed this manifesto:

As PR writer I will:

- keep PRs small

- use labels to indicate PR is one of many parts

- post PRs on Slack when opened

As PR reviewer I will:

- make an effort to review as soon as I get a second.

- approve, as long as it’s better than before

- prefer raising a new ticket or requiring a follow up PR to make it better over rejecting the PR

- favour suggestions over rejections, particularly when multipart indicated via label

Let’s look at this. The essence is: Pull requests need to be small and fast!

This is also in line with the Programmers Oath:

I will make frequent, small, releases so that I do not impede the progress of others.

But as we all know the problem with pull requests is often that they can sit in review for some time.

The larger the request, the longer the review

We want to get as close as we can to a head development approach where code easily goes into master/develop. We should aim for a continous flow of _good_code. We need to fight against long-lived feature branches because they are the root of all evil!

Therefore pull requests need to be reviewable quickly to get code merged fast. But this can only work with small pull requests! You won’t get a good review on a large PR and it will take ages to get it merged. Because of this, some companies have a limit on the number of lines changed for each PR. In general they should be less than 300 lines long, otherwise they are not reviewable anymore.

The longer the PR the more tiring it is for the reviewer

If reviewing is tiring, developers won’t want to review. But we need the team to review code as often as possible so don’t make it hard for them!

Give context

Make it easy for the reviewers to understand your changes.They will probably not be as familiar as you are with what you’re working on. Add a good description and some screenshots:

Prevent context switches

The sooner review comments are received, the less likely it is that a developer is already on to the next job and has to switch context back to the previous one. The longer it takes, the harder it is to switch context back and make the changes. So, make your pull requests as small as possible and create them as often as possible: at least ones a day! Or even more often! Rebase issues will become very rare if you follow this.

The reviewer needs to help too

If you only review pull requests once a day, the idea of opening multiple request per day will not work for your team. So review often! After every break, before you start a new ticket, after every pomodoro cycle or every time you open a pull request yourself. Our team introduced a cap on open pull requests similar to the WIP limit in Kanban. No one is allowed to open a pull requests if the limit is reached, first review others to clean up the queue!

Focus on what’s important

All code style should be checked by some automated task first — that’s not a task for a human being. CI should help take care of a significant amount of code check (static analysis: anti-patterns, complexity, potential memory leaks) and reviews can easily focus on logic and architecture.

Don’t be too serious

Pull requests are a discussion with your team members. Don’t make it a teaching session. Make suggestions don’t request them. Be friendly. Use emoji and GIFs to make the reader smile of your suggestions:

Reviews are feedbacks to your colleagues and there is positive feedback too, appreciate if something is well done.

Pull requests are not the place for long architecture discussions

Don’t over discuss on pull requests. It is too late anyway, the code is written! Use other outlets for that such as a daily/weekly developer meeting. Pull requests are in place to make sure that the quality level improves and to spot potential bugs and side effects. If you have juniors in the team, try using pair programming, don’t teach via pull requests as it will become frustrating.

If the code is better than before, approve it

If there is something that could be done even better, open an issue or ticket. Of course this needs a working culture that adresses tech debt continuously so that this ticket will not get buried in the backlog. This is much easier to prioritize if the pull requests is just a partial ticket. Another request will come in for sure soon that could address this immediately.

Don’t be scared

In a remote setup like ours, it can be scary when requests might be merged overnight — requests you never even had the chance to see or comment on. When a team grows this is normal. You can not control, check or know any line of code. Accepting this takes courage and trust!

PS: This article was first published here.