On code reviews
workA great PR - #
- Should be less risky. Should point out the risk(s) in the description, if any.
- It should take minimal review cycles.
- Is scoped properly. It should do one thing - bug, feature, enhancement, migration. Only 1. And this should be added as a label (or whatever practice one can follow to let the reviewer(s) know what it is)
- Is green when you ask for a review. Not red. If red, a reason is provided.
- Has a familiar reviewer.
- Should have these in description (whenever needed) (Codebases should have templates for bugs/features/chores, etc)-
- Set the context (what and why)
- Related tickets, other PRs, issues
- A checklist of what it does with ticked/unticked items (with commit SHAs)
- Pre and post-deployment steps (run migrations, add this env variable, run this rake task), if applicable
- Steps to take in case of a revert. (Assume that someone else might need to revert your PR)
- Screenshot(s) / Videos in case of UI changes
- Is easier to review
Great PR reviews - #
-
For non-trivial changes take time. "LGTM" after 3 seconds is not good enough.
-
Are kind and thorough. Don't review the person, review the code that's written.
-
Few changes are time-sensitive and the reviewer should know which ones are such. They'd try to introduce minimal friction in such cases.
-
Review the whole PR at once. Partial reviews lead to more iterations and churn, causing the team to ship later than necessary.
-
Shouldn't be about style. Linter should be used in CI.
-
Justify blocking - flaws, testing gaps, too big PRs, over-engineering, obvious readability gaps.
-
A practice that I tend to follow is to provide type of comment along with a PR comment. Eg-
"Question(blocking) : Does this logic make more sense in
xyz.rb
instead ofabc.rb
?""Suggestion(non-blocking) : We have a wrapper for doing this in
abc.rb
, it can be directly used here." -
Provide insights - what else is needed and possible solutions while blocking reviews for PR creaters.
- Next: On communication