...
The Author: The author of the PR has the responsibility and room to make sure the PR is clear and approachable to other team members so that they can give a confident review.
Provide context to what brought about the Pull Request
Provide a summary of the problem the Pull Request is solving
Provide a summary of the solution that the Pull Request provides
The Reviewer: The reviewer of a PR has the responsibility to unblock the work of the author. They do this by approving the pull request, or by requesting needed changes and providing explicit feedback when necessary. No matter which way the review goes, it’s crucial that the review is timely so that they don’t block and frustrate their team members.
Unblocking the PR author
Validate the code: Make sure that the code in the PR looks like it is doing what the author intended it to do. This includes ensuring that there is proper testing.
Provide good feedback, which should include the following:
Clear, actionable advice with examples of what (if anything) should be changed.
Reasons for why the changes have been suggested, with documentation or evidence that supports that point of view.
An impersonal tone, so the review does not feel like overt criticism.
Do all this in a reasonable amount of time
Provide clear instructions for the QA person to follow to validate the task if approved
QA: The person doing the QA review should generally be the person who interacted directly with the client to create the issue originally, as they have the best overall picture of what the ticket was supposed to accomplish.
Follow the provided guide to validating the business goals of the original ticket have been met
Provide clear guidance for the Reviewer if additional work is needed
Do all this in a reasonable amount of time.
The Team Itself: Good PR practices are helpful at an individual level, but to be most effective, the team should agree on and adopt a set of standards that the team can collectively hold PRs to.
Decide a reasonable time table
Decide what should be blocking vs non-blocking
Blocking
Bugs: This could be “This code will error when X happens. X happens often”
Untested code changes: How do we know if the code works if we can’t verify it?
Hard-to-Comprehend code: When a section of code is hard to read and hard to understand what it’s doing, it adds uncertainty to the code as well as a maintenance burden if it ever needs changes. We decided that’s a good enough reason to block code.
Non-blocking
Code styles: This is stuff like “could use .map here”. I call these “code flavors” because there is more than one way to do things and everyone has their own preference on what’s more “readable”.
Improvements: - This is stuff like, “What if we extracted this out to its own method/class?”
Opinions as Standards - Some people hold certain coding authors and their rules to a high standard. I love and take inspiration from some book authors, but I think that it’s important to see their recommendations as best practice opinions rather than rules to follow. Because of that, I find comments to follow those patterns as code styles and improvements and therefore non-blocking.
...