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, there is a provided link to an 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, one wants to make sure I was consistent with how other PRs were made to this repository. One of the suggestions is the old “when in Rome” rule. Some repositories even provide a template, which can be helpful, but this one didn’t. So, one should make each item discussed in the issue into a bullet item, checking-off the ones that were completed and noting anything that you’re not 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.
For 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 re-factorings 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 “re-namings” 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 struggle with visualizing layout changes or CSS tweaks. Include a screen capture of the before and 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
Become 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.
Be cautious and curious in order to lead a better engagement and ultimately, a better solution.
Here is a comment made on a PR to a tslint project. You can see how the person’s comment/feedback is acknowledged and further asks a clarifying question because of the impact it would have on so many files. This lets the reviewers know that you have respect and consideration for the size changes coming into their code base, and also that you want to be collaborative in finding the best solution.
https://github.com/palantir/tslint/pull/1738#issuecomment-261527450
Closing Thoughts
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?
