Pull Requests Volume 1: Writing a Great Pull Request


When I gave my talk at NSNorth 2013, An Educated Guess (PDF) about building better software, one of the points I stressed was about understanding people and working together with them in better ways. This means knowing where you are strong and weak, and where the people you work with are strong and weak, and acknowledging the collective goals you share: to make better software. This means knowing how to deal with other people in positive ways, giving criticism or making suggestions for the betterment of the product, not for your own ego.

I don’t think I expressed my points very clearly in the talk, so I’d like to take some time now and provide something of a more concrete example: dealing Github’s Pull Request feature. This will be a multi-part series where I describe ways to use the feature in better ways, with the end result being an improved product, and also an improved team.

This particular example deals specifically with iOS applications and while using GitHub’s Pull Requests (and assumes you’re already familiar with the general branch/merge model), but I hope you’ll see this could just as easily apply to other software platforms, and other forms of version control. This guide stems from my experiences at Shopify and at the New York Times and is more focused on using pull requests within a team, but most of this still applies if you’re doing open source pull requests to people not on your team.

Writing Great Pull Requests

Let’s say you’ve just completed work on some nasty bug (I’ll be using a bugfix as an example, but I’ll note where you might like to do things different for a feature branch), you’ve got it fixed on your branch and now you’re ready to get the branch merged into your team’s main branch. Let’s start from the top of what you should do to make a great pull request.

Check the diff

The first thing you’ll want to do before you even make your pull request is review the differences between your branch and the target branch (usually master or develop). You can do this with lots of different tools, but GitHub has this built-in as part of the “Branches” page where you can compare to the default branch. Even better, in the “New Repository View” they’re beginning to roll out, reviewing your changes is now part of the Pull Request creation process in the first place.

Here’s where you’re going to look for last minute issues before you present your work to a reviewer. I’ll be writing about exactly what you should be looking for in Volume 2 later on, but the basic gist is: when looking at the diff, put yourself in the shoes of the reviewer, and try to spot issues they might find, before the review even starts. This is sort of like proofreading your essay or checking your answers before handing in a test. If you could include animated gifs on your tests.

Cleanup any issues you spot here and push up those changes too (don’t worry, all changes on the branch will get added to your pull request no matter when you push them up).

Use a descriptive title

The title is the first thing your reviewer is going to see, so do your best to make it as descriptive as possible, as succinctly as possible. Don’t be terse and don’t be verbose, but give it a good, memorable title. Remember, depending on how the team works, the reviewer might have a lot on their mind, so making it easier to tell apart from other pull requests at a glance will make things easier for them to review yours.

Some teams assign ID numbers to all features and bugfixes from issue tracker “stories”, and if your branch has one associated with it, it’s also a good idea to include that somewhere in the title, too. This allows software integration between Github and your team’s issue tracker software so the two can be linked together. Also, including the ID number in your pull request title helps eliminate the chance of ambiguity. If the reviewer really isn’t sure which issue the branch relates to, they can always use the ID number to verify.

Finally, try to be explicit with the verbs you use in the title. If your branch fixes a bug, use the word “fixes” or “fixed” somewhere in the title. If it implements a feature, use the word “implements” or “adds”, etc. This tells the reviewer at a glance what kind of pull request they’re dealing with without even having to look inside. As a bonus, when using an ID number as discussed above, some issue trackers will automatically pick up on key words in your title. Saying “Fixes bug #1234” can cause integrated issue trackers to automatically close the appropriate bug in their system. Let the computer work for you!

The Description

The last and most important thing you need to do to write a great pull request is to provide a good description. For me, this involves three things (although I’m open to hearing more/others), loosely borrowed from a Lightning Talk given by John Duff at Shopify. In a good description, you need to tell the reviewer:

  1. What the pull request does.
  2. How to test it.
  3. Any notes or caveats.

1. What it does

This is the most essential part of the pull request: describing what the changes do. This means providing a little background on the feature or bugfix. It also means explaining, in general terms, how the implementation works (e.g. “We discovered on older devices, we’d get an array-out-of-bounds exception” or “Adds a playlist mode to the video player by switching to AVQueuePlayer for multimedia”). The reviewer should still be able to tell what’s going on from the code, but it’s important to provide an overview of what the code does to support and explain it.

The real benefit of doing this is it gives your team a chance to learn something. It gives anyone on your team who’s reviewing the code a chance to learn about the issue you faced, how you figured it out, and how you implemented the feature or bugfix. Yes, all of that is in the code itself, but here you’ve just provided a natural language paragraph explaining it. You’ve now created a little artifact the team can refer to.

As an added bonus, when writing it up yourself, you’re also taking the opportunity to review your own assumptions about the branch, and this might reveal new things to you. You might realize you’ve fallen short on your implementation and you’ll be able to go back and fix it before anyone has even started reviewing it.

You’re the project’s mini-expert on this mini-topic, use this as a chance to let the whole team improve from it.

2. How to test.

Some development teams have their own dedicated QA teams, in which case providing testing steps aren’t usually as essential, because the QA team will have their own test plan. If your team does its own QA (as we did at Shopify) then it’s your responsibility to provide steps to test this branch. That includes:

  1. The simplest possible way to see a working implementation of whatever you’re merging in.
  2. The most complicated possible way to see a working implementation.
  3. Tricky edge cases, especially ones whose intended behaviour differs from the main use case.

If your branch fixes something visual in the application, it might be a good idea to provide some screenshots highlighting the changes. If your branch involves a specific set of data to work with, provide that too. Do what it takes to make it easier for the reviewer.

Of course, the reviewer should be diligent about testing this on their own anyway (in steps I’ll describe in Volume 2), but when you provide steps yourself, you’re again reviewing the work you’ve done and possibly recognizing things you’ve missed, cases you’ve overlooked that might still need work once a reviewer checks them over. This is another chance to remind yourself of strange test cases you might have not thought about.

3. Notes and Caveats

The last section doesn’t always need to be included, but it’s a good catch-all place to put extra details about the branch for those who are curious. It’s also a great place to explain remaining issues or todos with the pull request, or things this branch doesn’t solve yet.

If you’ve made larger changes to the project, this might be a good place to list some of the implications of this change, or the assumptions you’ve made when making the changes. This again gives the reviewer more clues as to what your thinking was, and how to better approach the review.

Assign a Reviewer

If it makes sense for your team, assign someone to review the pull request. When choosing a reviewer, try to find someone who should see the request. Who should see it? It depends. If you’re fixing an issue in a particularly tricky part of the application, try to get the person who knows that area best to review it. They’ll be able to provide a more critical eye and find edge cases you might not have thought of. If you’re changing something related to style (be it code style or visual style), assign the appropriate nerd on your team. If you’ve got an issue that would reveal and explain the internals of a part to someone who might not know them as well (like a newcomer on the team), assigning them the pull request will give them a good chance to learn.

This doesn’t necessarily mean assign it to the person who will give you the least strife, because sometimes strife is exactly what you need to improve yourself and the code (if the reviewer is giving you strife just for their own ego, that’s another story which I’ll discuss in Volume 2).

Remember, if the reviewer finds issues with your code, it’s not personal, it’s meant to make the project better.

Not always applicable things

Some of the suggestions I’ve made will seem like overkill for some pull requests, and in a way that’s a good thing. They’ll make less sense for smaller pull requests, where the changes are more granular or uninvolved — and these are really the best kinds of pull requests, because they change only small amounts of things at a time and are easier to review. But sometimes larger changes just can’t be avoided, so that’s where these suggestions make the most sense.

Building Better Software by Building Software Better

These are tips and guidelines; suggestions for how to make it easier for the people you collaborate with. Once you think of Pull Requests as really a form of communication between developers, then you see it as an opportunity to collaborate in better ways. It becomes less about passing/failing some social or technical test, and more about improving both the team and the project. It’s a chance for all parties involved to learn something new, and do so in a faster way. It’s not about ego, it’s about doing better collective work.

In Volume 2 I’ll do the same from the other perspective: Giving Great Pull Request Reviews, to see how it looks from the other side.

Speed of Light