5  Writing review comments

5.1 Courtesy

In general, it is important to be courteous and respectful while also being very clear and helpful to the author whose code you are reviewing. One way to do this is to be sure that you are always making comments about the code and never making comments about the author. You don’t always have to follow this practice, but you should definitely use it when saying something that might otherwise be upsetting or contentious. For example:

Bad: “Why did you use threads here when there’s obviously no benefit to be gained from concurrency?”

Good: “The concurrency model here is adding complexity to the system without any actual performance benefit that I can see. Because there’s no performance benefit, it’s best for this code to be single-threaded instead of using multiple threads.”

5.2 Explain why

One thing you’ll notice about the “good” example from above is that it helps the author understand why you are making your comment. You don’t always need to include this information in your review comments, but sometimes it’s appropriate to give a bit more explanation around your intent, the best practice you’re following (like the design or style guide), or how your suggestion improves code health.

5.3 Giving guidance

In general it is the author’s responsibility to fix a PR, not the reviewer’s. You are not required to do detailed design of a solution or write code for the author.

This doesn’t mean the reviewer should be unhelpful. In general you should strike an appropriate balance between pointing out problems and providing direct guidance. Pointing out problems and letting the author make a decision often helps the author learn, resulting in higher quality PRs in the future. It can also result in a better solution, because the author is typically closer to the code than the reviewer is.

One example of this is to simply point out a section of code that confused you, or which took a long time to understand (like with a why code comment). This isn’t a direct call to action, but prompts the author to take another look at the code in section to see if it can be further simplified or clarified.

Another way to demonstrate an issue is to provide the author a reprex that demonstrates that there is still a bug in their PR. The author can then decide how to resolve it.

However, sometimes direct instructions, suggestions, or even code are more helpful. The primary goal of code review is to get the best PR possible. A secondary goal is improving the skills of PR authors so that they require less and less review over time.

Remember that people learn from reinforcement of what they are doing well and not just what they could do better. If you see things you like in the PR, comment on those too! Examples: author cleaned up a messy algorithm, added exemplary test coverage, or you as the reviewer learned something from the PR. Just as with all comments, include why you liked something, further encouraging the author to continue good practices.

5.4 GitHub suggestions

For very small tweaks, like typos or additions to comments, GitHub provides a feature known as suggestions. As a reviewer, you can add suggestions using the workflow shown below, and the author can accept them directly in the GitHub UI and commit them into the PR (which they can then pull locally to get everything up to date if they have more work to do).

Note that if your suggestion touches documentation, then the author will have to merge, pull locally, and run devtools::document(). If you notice this situation, it is typically polite to mention to the author that they will need to document() locally after committing the suggestions, because that is particularly hard to remember.

As a reviewer, you can add multiple suggestions and the author can batch multiple suggestions into a single commit. The author must first hit Files changed to switch to the viewer mode, at which point the suggestion will show up with an additional option - Add suggestion to batch.

After the author has finished adding suggestions to the batch, they can Commit suggestions en masse using the button at the top of the screen.

5.5 Label comment severity

Consider labeling the severity of your comments, differentiating required changes from guidelines or suggestions.

Here are some examples:

Nit: This is a minor thing.

Optional (or Consider): I think this may be a good idea, but it’s not strictly required.

FYI: I don’t expect you to do this in this PR, but you may find this interesting to think about for the future.

This makes review intent explicit and helps authors prioritize the importance of various comments. It also helps avoid misunderstandings; for example, without comment labels, authors may interpret all comments as mandatory, even if some comments are merely intended to be informational or optional.

5.6 Approve with comments

When you’ve finished leaving review comments, GitHub provides a number of options for finalizing your review. The most common kind of PR review within the tidyteam is known as “approved with comments.” It involves leaving a few minor comments on a PR, while also hitting the Approve button on the GitHub review UI:

This gives the PR author permission to merge the PR as soon as they have addressed the comments without needing to request another round of review. This is done when either:

• The reviewer is confident that the author will appropriately address all the reviewer’s remaining comments.
• The remaining changes are minor and don’t have to be done by the author.

Approving with comments is largely about the experience of the PR author. Close-knit collaborators often approve each other’s work after just one round of review, because both parties have significant experience working on the package and the PR is likely to be mostly correct from the beginning. For understudies and external contributions from new contributors, it is common for reviewers to use the Comment option instead, which is a signal to the author that they should request another round of review when they have finished addressing your comments. It is important to remember to be patient with new developers; they are going to need multiple rounds of detailed reviews early on, but putting in this extra effort up front tends to lead to faster PRs in the future as the author learns more about what is expected from their PRs.

5.7 Requesting changes

GitHub also includes a third option when submitting a review, Request changes. This is typically reserved for more drastic changes that absolutely require another round of review. This option is rare to see in close-knit collaboration, but is somewhat common when the author is an understudy and with external contributions, where the author likely doesn’t have as much expertise as the reviewer.

5.8 Finishing off an external contribution

Sometimes when we receive external contributions, we decide that rather than leaving comments for the external contributor, it is more efficient for us as the reviewer (and usually package maintainer) to just finish off the PR and merge it for them. This typically happens when we come back to work on a package after letting it lie dormant for a few months, and we find that we have a few PRs from external contributors. Sometimes these PRs can sit for awhile while no one is actively working on the package, and it might not make much sense to leave comments for the PR author weeks after they sent the PR in. Using usethis::pr_fetch() and usethis::pr_push(), you can instead just take over the PR, fix any minor issues, and merge it in. Make sure that if you do this, you also thank the author for their contributions!

It is also appropriate to inform a PR author that you’ll finish off a PR if you have already gone through 1-2 rounds of PR review and there are still a few minor edits that need to be made before the PR is ready to be merged.