Pull Request Best Practices

Pull requests (PRs) are a way for us to suggest changes to a codebase, and allow the owner and other members to suggest any edits, additions or removals before it’s in a fit state to contribute back to the main codebase. They are relatively easy to get started with, but just like any reasonably complex tool the deeper you go the easier it is to misuse. This blog post aims to highlight to you some of the better practices you can and should use that we’ve learnt over the years to help catalyse team collaboration.

Why pull requests, anyway?

Since the very first piece of code was shared between two people, the difficulties with safely introducing updates and changes to a codebase became apparent very quickly. If the two sharing code were not careful, they could easily ruin days or even weeks worth of work with a simple slip up. Even worse when you add a few more people to the team you can start losing work when resolving conflicts and spend duplicated effort across the codebase (you do modularise and DRY your code, right?). Commands like git am came about to help developers share patches to others via email which was an improvement, but you can see where this turns into email chain hell. Pull requests are a natural progression that enables developers to make sharing and patching even easier.

The Practices

Two-thumb approval 👍👍

Sort of a practice, sort of a convention – originally I only wanted to share 3 practices but this is very important. Github has reworked the user journey of reviewing and approving a PR since I started using it, and allows you to explicitly “approve” or “request changes”. The options allow you to prevent merging a PR unless it’s been approved, but there is no minimum or maximum number of approvals. Internally we like to have two approvals from different team members before something is ready to be merged (with the exception of pair programmed PRs, but that’s a topic for another day). This helps to spread the knowledge of the codebase, encourage cross-pollination of best practices between senior and junior members and is a lot easier to do than to accidentally deploy a mistake to production. Back in the day you would add a thumbs up emoji (👍) to indicate your approval, or on Bitbucket (unless they’ve updated that too since I last used it).

Create it early, update it often

When you first start working on a new feature, addressing a bug or reworking a piece of code it’s always best to open up a PR as early as possible and update it regularly. By creating it early you establish a starting point, signals to the others in the team what you’re working on and prevents duplicate work from being done. In a professional context and smaller teams this tends to be less of an issue, but certainly for public and open source projects it can be disheartening to put in a lot of work and effort into something only for someone else to submit a PR and have it accepted before you do. Or worse, your work being obsolete.

Updating your PR often actually serves a dual-purpose. Firstly it helps others to see how your work is progressing, offer any suggestions or hints and allows you to open up your work for early review. This is especially useful for younger developers or those at the beginning of their career. Secondly it helps yourself to have a “save point”; should you lose your machine or accidentally delete your project folder you can simply git clone my-project; git checkout feature/my-feature and you’re back on track. When you have a quality commit history, wonderful bonus for more advanced git users is that using git cherry-pick and git rebase become ridiculously easy to use and master when the need arises.

Inform others what you’re trying to do

As well as creating your PR early and making regular commits, you can and should inform to the rest of the team what you’re trying to do using the description box. Do this even if you have a JIRA ticket or a Trello card you’re working against. Again this serves a dual purpose to your team. First it signals to the rest of your team what you’re trying to accomplish – more experienced contributors may be able to suggest an easier route than what you originally had in mind and helps others plan their work to avoid clashes. Even with a team of two people on a smaller codebase, you can very easily get conflicts in your code if you’re not careful.

Secondly it leaves a paper trail that allows you to go back in time and see why you implemented something a certain way and roughly the route you took. I’ve had the pleasure of learning this the hard way more than once, wondering why something was done a certain way or why it even existed. It was invaluable to be able to search for a JIRA ticket, trace it back to a PR and see what we were trying to do, any clashes with any other PRs at the time and the state the codebase was in. If you’re not sure how to write a PR description, have a look at our technical-culture repo as starting point you can remix for your own team.

On size – treat them like food

Just like with food, to enjoy working with pull requests best you need little and often rather than one big one once in a blue moon. A single large PR seems sensible at first to capture a single ticket/card/task, until it takes hours or days to have yours reviewed and worse yet you need to address an avalanche of comments. The longer it’s up the higher chance it has to become outdated and create conflicts with work from other team mates, slowing down overall productivity. By keeping them small and self-contained you make it easier both for yourself to keep track of and reason about your open PRs, and any reviewers will be more able to quickly load the “state” of your PR in their heads and swiftly help get your changes through.

As a rule of thumb your PR should normally be less than 1000 lines different (both additions and subtractions) across around 10 files. Reviewers should only need about 30 minutes tops to go through and make any suggestions and improvements to your code and approach. Of course this is simply a rule of thumb and not a definitive guide – there are times where a larger PR is unavoidable and that’s okay. If you feel your ticket or task will get larger than this, try and find a natural “break point” of modular code and split it into two, or even three PRs and make that clear in your comments and descriptions. That’s okay!

Example: breaking up your feature into multiple PRs

First let’s make a branch and begin working on it…

We’ve done plenty of work but the branch is getting pretty heavy. Let’s tidy up our code and push it up to Github and get our PR ready for review:

Nice! Last step is to branch off and continue working

Don’t forget to update your develop branch and rebase your second feature/registration-ii branch once your first PR is merged:

Summary

In this post I showed you a flavour of what the world is like without pull requests, hence why we like using them, and four practices that can help you and your team manage your codebase more easily. Two-thumb approval is a simple but effective tool to keep quality and cross-pollination of knowledge up; by creating one early you avoid coding yourself into a rabbit hole and can get early feedback; by updating it often you create a “save point” for yourself and make life easier for advanced tools like cherry-pick and rebase; by having a clear description you help others help you earlier on and leave a paper trail for your future self or the future maintainer of the code; and finally by keeping them bite sized you allow yourself and your reviewers to load and fit your PR in mind, making everyone’s job easier and it’s okay to have multiple PRs against a single ticket.

Random Trivia

According to a question on Stack Exchange the first sighting of a Version Control System was in 1972 called the Source Code Control System (SCCS).

Read this…