On code reviews

23 Jul 2023 work

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

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 of abc.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.