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:

1. Provide context
2. Make it as small as possible, but not any smaller
3. Take screenshots
4. 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

Pull Request (PR) reviews are a critical aspect of collective code ownership. PR reviews reveal to your coworkers, your problem solving methods, thought process and coding style. Pull Requests also give your teammates insight into the problem you’ve been tackling and potentially visibility into a part of the code they are not familiar with.

Making a pull request can be empowering, but it can also make you feel vulnerable. Exposing your hard work to others can be intimidating. Will it be accepted? Will others think my code is garbage? Am I smart enough to even be making these types of changes? Whether you are working on an internal or open source project, just remember that we’ve all been there before. The projects and teams that you want to be part of will welcome your contributions, and will work with you by giving you meaningful feedback. Most PRs don’t get merged without a comment or request for a change, don’t get discouraged!

When it comes to reviewing PRs, your role as a developer changes from “problem solver” to “solution reviewer”. It’s troubling how this seemingly simple switching of hats causes some to take on a completely different personality. Of course, there are also those that show their true colors, for better, or worse. I’ve found that negative PR reviewers generally fall into one of five types:

1. The Know-It-All
2. The Comment-Golfer
3. The Machine-Gun-Sprayer
4. The Manual-Linter
5. The I’m-Gonna-Get-You reviewer

Photo by Chase Clark on Unsplash

The Know-It-All goes into extremely long comments to deliver their diatribe on a particular topic. It’s usually that person that doesn’t miss an opportunity to rant or ramble during a team meeting. Sometimes there are useful nuggets in the wall of text, but those can be hard to extract. This person usually means well, but comes across as “I’m better than you” and leads to a lot of frustration because the comments are difficult to respond to.

On the other end of the spectrum is the Comment-Golfer. Code golf is a competition where “participants strive to achieve the shortest possible source code that implements a certain algorithm” (via Wikipedia). Imagine earning points for leaving the shortest possible comment. “Use reduce” or “singleton pattern”. Or maybe you won’t even get actual words, and you’ll just get links to blog posts (apparently PR comment-golf doesn’t count links as characters). So you’ll see a link to a stackoverflow answer, or a Martin Fowler article and have to try to figure out the intention of commenter. These types of comments usually benefit from more context and a splash of sentence structure learned in 3rd grade. (Side note: I’m a big fan of Fowler’s writings, my point here is that context is everything)

Photo by Zhang H on Unsplash

The third type is what I call the Machine-Gun-Sprayer. This is another person who thinks someone is keeping score, and that each comment is worth a point. They will post a dozen comments on your PR pointing out the same thing over & over (i.e. “You should use map instead of forEach” or “Need a space here”). Combined with the Comment-Golfer, this person racks up points in a game where, unfortunately, everyone loses.

The fourth type is one you probably recognize: the Manual-Linter. This is the person that points out all the inconsistencies your PR has with her preferred coding style. In fairness, sometimes it’s the team’s agreed-to coding style, but they’re the ones that aren’t codified in linting rules. Instead of taking the time to research and write a lint rule, the Manual-Linter prefers to leave comments about spacing, braces, indents, new lines and “margin: 0px” vs. “margin: 0” .. on every single PR!

Photo by Adi Goldstein on Unsplash

The fifth, and most toxic type is the I’m-Gonna-Get-You reviewer. This is the person that no matter how big or small the PR, always leaves negative, sometimes condescending comments. This happens because the reviewer does not trust, or worse, doesn’t “like” the person who made the code change. This is unacceptable behavior, especially in a corporate/team environment. Unfortunately it’s not so simple to mitigate. These types of comments can divide a team and cause completely unnecessary distress amongst it.

The worst part of these personas is that most of them would be upset if the tables were turned and someone commented on their PR in the same way! It’s mind-numbingly simple to just leave comments that you would appreciate people leaving on your Pull Request. It’s the old saying “Treat others the way you want to be treated”. Nothing new here. This is kindergarten stuff.

How do we deal with these personas?

While there is no one-size-fits-all solution here, there are some things that can be done to make reviewing and receiving comments on Pull Requests a little bit easier, and friendlier.

I propose 3 solutions to handling these personas

1. Collaborate more
2. Dedicate time
3. Make better PRs

Collaborate more

In my experience, the Know-It-All usually comes from a good place, they want to help. Encouraging this person to get involved earlier (grooming/planning) or to pair program is essential. This not only helps the Know-It-All imbue his thoughts and knowledge, but spreads domain knowledge to the rest of the team. The Manual-Linter may not know how to write a lint rule. Take it upon yourself to sit down with them and work through it together. The benefits of codifying rules into an automated system should be obvious. Just be sure the team agrees with the changes, or urge the Manual-Linter to stop leaving comments that don’t have the support of the team. I am a huge proponent of automation. Automate everything you can. If you have face strong resistance against certain rules or tools, talk them through. Find out what’s behind the resistance and try to find a compromise. Be incremental, start small. Anything is better than having a live, breathing human checking for semicolons and whitespace!

The I’m-Gonna-Get-You reviewer is a bit unique in this list. This one isn’t going to get solved by tooling, but by communication. Before teammates become sworn enemies, it’s important to talk. I’m not suggesting couples counseling, but something pretty close. Without broaching the topic, it will only escalate and drag down the entire team. As a team member, I expect that you’ve picked up on this behavior either through witnessing it first hand, or hearing about it from concerned teammates. Either way it’s important to get the two sides talking. If you aren’t comfortable approaching either of the people involved, engage your team lead. Involve management only if necessary, but certainly do so if you feel the situation is out of control, the resolution isn’t to your satisfaction or the problem persists.

Dedicate time

Photo by rawpixel on Unsplash

Part of the problem may be that not enough time or priority is given to reviewing your teammate’s code. The Comment-Golfer and Machine-Gun-Sprayer may just go in for the “quick win” which unfortunately leaves a bloody trail of teammates behind. This is an easy way to make it look like a proper review was done (“Look at all these comments!”). A reminder that “this is your code base too” along with allocating enough time are potential ways to resolve this part of the problem. Personally, I like to do my PR reviews first thing in the morning (before most get in), and know some people literally block their calendar to do it. These are usually team leads that have the most knowledge of the system and can help see patterns and reduce duplication. But, remember: this is a team effort, so everyone must do their part.

Make better PRs

Another substantial part of the problem can be attributed to the PR itself. Reviewing code is not a simple task, it requires a basic understand of the goal of the PR and knowledge of the system. So guess what happens when you drop a 34 file, 1200 line change on your teammates? The same thing that happens to you when you see it … you groan! Smaller PRs are easier to review, and while they tend to get more comments, it’s usually better to be heavily scrutinized than ignored. It’s 100x better than the blind “Approve” click that typically accompanies a large PR.

Besides “smaller”, PRs should also provide enough context for the reviewer, who may not know what you’ve been working on, to quickly understand the scope of your change. This could be as simple as a link back to the original defect or story. If you are changing the UI, screenshots are always appreciated - a before & after comparison can go a long way. And don’t be shy, if you aren’t sure about something, comment on your own code! I’ve found it extremely helpful to comment on my own PR with something like “I’m not sure this is the best way to do this”, especially on a function or a loop that I was able to make work, but just felt less than ideal. Making these kinds of comments shows that you tried, and shows that you are open to suggestions from the reviewers.

Conclusion

Writing code is hard, reviewing other people’s code is even harder. We’ve looked at different types of problems that can arise during Pull Requests and potential solutions to resolving them. Next time we’ll take a look at what goes into making a good PR comment and provide some examples.

What other personas have you witnessed in the PR review process? How did you and your team handle those situations?