How to comment on a Pull Request

December 28, 2018 | By Brian | No Comments | Filed in: work.

Since Pull Requests are so important, and it’s easy to make unhelpful comments, how do you make a good PR comment and what does one look like?

It’s important to remember that as a reviewer, you are in a position of power. Usually that means the power to accept or decline the Pull Request. The requester is essentially at your mercy. Will you be ruthless? Or will you be kind?

In my experience, it’s the people that show compassion and are willing to guide the requester that get the most useful and productive work done. The most important piece of advice I can give you is to treat each Pull Request as an opportunity for a collaborative conversation.

The tab in GitHub literally says Conversation!

Let’s take a look at a few examples of what I consider good PR comments.

This comment was made on a Pull Request I created to the npm cli. It could have easily just said “Needs work” followed by a bulleted list of things that were wrong or needed tweaking. While it’s true that the PR “needs work”, a two word comment doesn’t emanate the feeling of collaboration. zkat appreciated the PR (“Thanks”) and acknowledged the time investment (“taking the time to do this”) I made on this PR. The words “super helpful” are a positive reinforcement that made me want to make any requested changes. Also note: GitHub uses the words “requested changes” and not “requires changes” or “demands changes”. The right word makes all the difference.


Here’s another from zkat on a second PR made to the same repository. Starting off with an apology is a good way to diffuse the oft seen “why isn’t my PR merged yet?” question. In truth, I did kinda ask that question, albeit slightly more friendly by asking if there was anything more that could be done. This was only after the PR had sat without comment for about 2 months. I get it, people are busy. It’s good to make sure your collaborators know that you are aware of that fact. But a nudge after a while won’t hurt.

After apologizing, zkat makes a request and asks if I can write a test… and provides the reasoning behind it. How can someone say “no” to a request like this? They answer is: they can’t. Even though I have yet to write one (over a month since asked), because you know, busy and stuff. I will get to it though, and I am happy to, in no small part because of zkat’s efforts to collaborate.


In this example, alexwolfe uses words like “cool idea”, “thanks so much”, and “great idea”. Even though the PR didn’t get accepted, it’s clear that kennethormandy’s effort was appreciated. alexwolfe also goes into a detailed explanation that includes a peek into future plans for the CTA and commits to trying to find other ways to include the main idea of this PR. Even if it doesn’t happen, alexwolfe leaves the door open for potential future PRs in this area.


This comment by benlesh is another good example of how to write a good comment even though he’s decided to close/decline the PR. Closing without accepting is essentially rejecting the creator’s request. This can obviously become contentious and it happens fairly often. But, by giving its creator kudos and showing appreciation for their efforts, it was avoided. I love the fact that benlesh offered to help bhaidar refine it as a blog post and “spread it”.


By using more than 2 words, Brocco avoids falling into the trap of the Comment-Golfer. I also appreciate the use of the words “Looks like” and “does not seem to be”. To be honest, there isn’t much more that could be written, but by avoiding “Delete this” or “Unused”, Brocco is indicating what he thinks should be done, without demanding it. The words also indicate that he’s not 100% sure that he’s right. While some may look at this uncertainty as a weakness, I see it as an invitation for someone to provide more information to the contrary.


Keep it simple, but be thoughtful. Show appreciation for the time and effort. Clearly identify things that need more work. Be willing to collaborate to make it better.

What rules do you follow when reviewing a Pull Request? Let me know on Twitter @olore

A Good Pull Request

December 27, 2018 | By Brian | 1 Comment | Filed in: work.

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.

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.

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.

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