PR Review Guidelines
Our responsibility, when reviewing the PR of a team member, is to support them in shipping the best code they possibly can. To do this effectively, we need to approach code reviews with empathy, humility, and understanding. We must trust the PR's author to have the agency to act upon our suggestions as they see fit.
This isn't your code
PR authors often find it difficult to not see their code as a reflection of their self-worth or accomplishments -- the industry repeats the mantra of "you are not your code" and that is correct!
While we all strive to write what we respectively think is the best code possible, much of the implementation is very subjective. What Sally thinks is perfectly fine code, Bob may balk at. That is okay! While Bob should voice his concerns, it is dependent upon Sally to act upon those concerns as they see fit. It is Sally's code, after all, and Bob needs to remember that it isn't his code. Bob's suggestions may go ignored, but that is in no way a reflection upon Bob -- "you are not your code (review)".
Prioritizing feedback
When reviewing a PR, your feedback should be prioritized to align with the goals of the team.
- Accuracy
- Does this PR resolve the issue at hand effectively?
- Is there an edge case that will cause a problem?
- Regression-less
- Are legacy data and configurations accounted for?
- Does this implementation negatively impact performance in a meaningful way?
- Confidence
- Will we be reasonably confident this implementation will continue to work in the future?
- Tests? Documentation?
- Maintainability
- Is the implementation clear, concise, and easy to understand for others?
- Verbose simple code is better than concise clever code
- Don't Repeat Yourself when reasonably possible
- Consistency
- Is this code reasonably align with similar implementations and code-style within the project?
When to Comment, Approve, or Request Changes
The verdict of your review has a lasting impression upon the velocity of the PR. Be mindful of your feedback and how it correlates to the goals of the team. Your goal, as the Reviewer, is to Approve the PR, unless you can find sufficient evidence not to Approve it.
Request changes: When you find a bug or critical flaw in the PR.
- Example: You've discovered a bug or edge case
- Example: Functionality is duplicated elsewhere in the project
Comment: When you have a question about the PR which prevents you from proceeding with your review. Once your question is answered, you will commit to continuing with the Review.
- Example: "Could part of this implementation be replaced with [this] existing functionality provided by the framework or elsewhere in the code base?"
- Example: Review instructions are unclear or need more explanation
Approve: You're reasonably confident that the PR aligns with the team's goals without defects or major imperfections. Suggestions are still okay!
A note on code-style
Code-style can be very subjective. To eliminate the ambiguity around code-style, our automated linters and formatters will enforce mandatory changes. Never block a PR based on code-style which is not highlighted by this automated enforcement. Comments and suggestions are still okay, but are subject to the author's desire to implement the recommendations. Lack of agreement of a given code-style should be addressed at the project level outside of the scope of another PR.
Ultimately, code-style enforcement, and the guides they're based upon, are suggestions to help us right more consistent and uniform code. They are not divine truth. Exclusions are acceptable.
Ultimately, code-style enforcement, and the guides they're based upon, are suggestions to help us right more consistent and uniform code. They are not divine truth. Exclusions are acceptable.