Each actionable comment you make on a Code Review should be prefixed with a label.
There are four labels that should be used. They describe what the contributor needs to do in order for the comment to be resolved.
Labels should be the first thing in the comment and formatted as follows:
[fix]It is something that must be addressed before merge.[plan] @usernameIt is something that must be addressed, but does not block a merge. These comments need to explicit state who needs to do what. That person needs to have done the action before the comment can be resolved. Examples of actions may include "Create an Issue..." or "Organize a meeting to discuss..."[consider]It is something that is not required for merge but should be considered. The contributor should affect that change, or, reply with a reason for not doing it and resolve the comment. Considerations can also use a secondary label such as[nit-pick],[minor]to weight the importance of the consideration.[question]It is something that needs clarifying before an action can be determined.
Praise and other "no action required" comments do not need a label.
Avoid the tempation to create new labels unless a new action is required. More labels generally leads to more confusion.
Origin Story #
An important part of collaborative software development is the code review. This process generally results in a reviewer needing to leave a comment of some kind.
It is important when writing feedback that the recipient understands what the expected course of action is. This is often easier said than done.
We discovered numerous different approaches to this challenge, and explored the use of several. From our experiences publicly developing an open source library, we found they sometimes had the potential to create more confusion rather than less.
A common sticking-point in our trials was the requirement to memorize a particular set of labels, along with their associated meaning and required outcomes. Frequently the needed action was not clear from the label itself. Some terminology was also prone to differing interpretation depending on the individuals cultural background. This created a notable barrier to adoption, and could be confusing for new developers.
We ended up deriving and adopting this concise, literal-as-possible alternative, centered around imperative action labels. It has proven easy to learn, valuable in practice, and well received by developers.
Design Goals #
- Intuitive when ready by first-time contributors.
- No ambiguity as to the action to be taken.
- Minimal terminology to learn.
Examples #
[fix] The `tmp_name` variable isn't used anywhere.
[plan] @tom Thanks for fixing this, please can you make a ticket
to update the onboarding videos?
[consider] Not using the ternary operator might make it easier
to follow the logic (especially once we've slept and forgotten
everything).
[consider][minor] "instead of" might be a better phrasing as our
user-base is less technical.
[question] What is our minimum Python version? Are we able to
use the recently introduced Wallrus operator (:=)?
This solution is genius!! Awesomesauce. ❤️
Other Considerations #
- Use the suggested edit functionality wherever available for minor changes.
- When making critical comments, try to include alternatives.
- Try to explain the 'why' as well as the 'what'.
- Always consider the relative experience of the contributor in the specific domain in question.