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.
https://github.com/npm/cli/pull/32
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.
https://github.com/npm/cli/pull/61
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.
https://github.com/angular/angular.io/pull/11#issuecomment-84097728
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.
https://github.com/ReactiveX/rxjs/pull/4375
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”.
https://github.com/ReactiveX/rxjs/pull/3538
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