Pull Requests are the backbone of open source software development. They allow contributions by anyone, from anywhere. PRs are also a vital form of communication, even within a localized development team working on proprietary software. What makes a good Pull Request? Let’s break it down into 4 rules of thumb:
- Provide context
- Make it as small as possible, but not any smaller
- Take screenshots
- Ask for assistance
Provide context
Providing context is an important first step in guiding the reviewer. Use this as an opportunity to explain why you are making this change. This can be as simple as referring to the bug/defect/issue number, and as detailed as necessary to describe your change.
In this example, I provided a link to the original issue on the npm.community site and linked directly to the source of pnpm that was referenced in the original issue.
https://github.com/npm/cli/pull/32#issue-204418739
In this more complicated example, I wanted to make sure I was consistent with how other PRs were made to this repository. One of my suggestions is the old when in Rome rule. Some repositories even provide a template, which can be helpful, but this one didn’t. So, I made each item discussed in the issue into a bullet item, checked off the ones I completed and noted anything I wasn’t sure about.
https://github.com/npm/cli/pull/61#issue-211591081
Make it as small as possible, but not any smaller
Everyone agrees that smaller PRs are easier to review. Sometimes it’s just not possible to make a very small change, so here’s some practical advice: don’t do more than necessary. If you need to stray from the core of the change you are making, separate it. Here’s an example - Imagine you are adding a path through the code to handle a new requirement. Along the way you realize that some of the variables and functions could use better names, but changing those means that you now need to update a bunch of files in another area of the code. STOP! Don’t make that change! Or at least, don’t bunch it together with your other changes. Instead, make the rename change in its own commit, and probably in its own Pull Request too. In the PR you can explain why you are making the change and how it simplifies the real change you want to make easier.
This same strategy should be applied to most whitespace and refactorings you want to do during the course of implementing a new feature or resolving a defect. Be considerate of the reviewer’s time. There is nothing more frustrating than hunting through all the changes looking for the actual change and bumping into whitespace and renamings spread across many files.
Take screenshots
So you’re working on a story that affects the UI? Maybe you are fixing alignment in IE11, or adding a new interstitial modal when the user clicks a button. The code will get reviewed as it always does, but many people (like me) struggle with visualizing layout changes or CSS tweaks. Include a screen capture of the before & after. It’s usually pretty easy to get a before shot - just use the QA or production environment. Then for the after shot, use your local server. Both GitHub and Atlassian BitBucket allow you to paste images, so you can literally SHIFT-CTRL-CMD-4 (OSX) to copy a section of the screen to your clipboard, then CMD-V to paste it into the input box of your PR description.
Another incredibly helpful option is to use an application like GIPHY Capture to record an animated GIF that can be added to your PR. These are great for when you want to show an animation or a sequence of steps. Let’s face it, it’s a pain for the reviewers to fire up their Windows vm to try out your change that resolves an Edge problem. Make their life easier by including an animated GIF that shows exactly what changed right in the Pull Request!
Ask for assistance
I am a big fan of the quote “it is better to beg for forgiveness rather than ask for permission”. In so many instances, not just in software, this rule helps save time. But making too big a change in your PR may be received poorly, especially when you are not a regular contributing member. I’m not advocating “playing dumb”, but being cautious and curious can help lead to better engagement and ultimately a better solution.
Here is a comment I made on a PR to the tslint project I made a few years ago. You can see how I acknowledge the person’s comment/feedback and ask a clarifying question because of the impact it would have on so many files. This let’s the reviewers know that I have respect and consideration for the size changes coming into their code base, and also that I want to be collaborative in finding the best solution.
https://github.com/palantir/tslint/pull/1738#issuecomment-261527450
Conclusion
I am hopeful that these points help you make Pull Requests that are more quickly reviewed and accepted. What other things do you like to do in your PRs? What kinds of things would you like to see more of in PRs that you are reviewing? What would you like to see less of?
Let me know on Twitter: @olore