PR Comments

I’ve talked about what makes a good commit message. They’re important markers left for future developers to understand what changed and why as they look back to understand why things are the way they are. They’re also important in the moment to the people who are reviewing the code. They explain the purpose, expected changes and interactions, and potentially, what won’t change.

That’s important information when you’re reviewing a PR for a bunch of reasons. One of them is that it informs what you comment on. Any issues you see during the review should be noted, but that doesn’t mean they’re PR comments. Let’s say there’s a PR on some library function and you notice during the review that there are some uses of the method that don’t handle the returned error properly. You should definitely make a note, probably a ticket for the right group(s), but those fixes most likely shouldn't be part of the library change PR (unless the change is to how errors are reported). On the other hand, if you notice that some of the new code is a lot like other code in the library, that should be a comment on the PR.

Which leads to another way that the commit message helps inform how you make the comment. Generally speaking, PR comments come in 2 flavors, blocking and non-blocking. Blocking comments are those that the reviewer feels need to be fixed AND re-reviewed before it’s OK to land the change. Pretty straight forward.

It’s the non-blocking ones that can be more interesting. They can range from inquisitive/informational to “this needs to be fixed but I trust the author to get it right”. The challenge then is to make sure the author of the PR (one of the readers of the comment) understands the commenter’s intent. The easiest way to do that is to be explicit. Identify what kind of comment it is and what the expected response (if any) is.

In a larger PR, or where reviewers with different focuses are looking it might be helpful to add some identifiers. Maybe the comment is security related, or is particularly important for a specific use case. For example, a comment related to calling the method in a CI process vs a production service or a CLI app. Noting the difference will help bring awareness to the use case and possibly uncover additional issues.

Another important thing to keep in mind is that although comments are attached to a specific line of code, they might apply to a block/loop/method/library/etc. That means, particularly for the larger scoped comments, a one line subject can provide some context and let the reader know what’s coming. And if you need to provide context, you’ll want to provide the detail.

Which gives us a nice template to use for PR comments. Something like:

<label> [optional identifiers]: <subject>
<details>

In practice, this might look something like

nit: Misspelled word in error message, “thier” should be “their”

Which would be a blocking change that is expected to be fixed before land, but doesn’t need re-review. Or maybe

Suggestion: This logic is roughly duplicated in methods XXX, YYY, and ZZZ
A future PR be considered should refactor the logic into one place

Indicating a future task to decide if there should be a refactoring, and then in that case a new PR should be made to refactor. The existing PR should NOT be delayed/extended to include the refactoring.

Check out Conventional Comments for more examples and suggested labels/identifiers.