Just recently I've been reflecting on practices and things that have or have not worked well for me as a developer at Automattic and I wanted to share a couple of things that I have perceived in my behaviours and the behaviours of others around me, that positively impacted PR reviews. These are things that if I could print them on a check list for every dev to read as they approach a PR review I would. I bet you can find many instances where I didn't live up to this too! This is aspirational for myself as well, a goal to continually strive towards.
- use github pr comments to tell people what i did and why. received universally positive feedback on this, why don't we all do it? Sometimes a code comment does not belong, but spending time helping the reviewer get context is valuable
- when reviewing any kind of UX behaviour, give me repro steps, give me a video of the behaviour. we really slow each other down async when we dont give enough detail of bugs we found or how we found them.
- Nit picking: nit picking is fine, I do it all the time, and we often prefix with "nitpick" to remind the developer that its a small thing, but I want to be wary of my reviews being heavy on nit picks and light on real feedback. As a challenge to yourself when reviewing any PR: can you give meaningful review that doesn't fall into the nitpick category. We are all hungry for real and valuable review. I don't believe our nitpicks move the mark on quality, but considering: performance, security, software architecture, SOLID principles absolutely will. Prioritize this kind of review even if it requires stopping and putting your "thinking cap" on. Nitpicks are generally easy and therefore low value to the author.
- Write great descriptions. As a reviewer: read all the detail in the description. If there are links to follow, follow them, read the back story. Across the async divide we have limited ways to give each other the nuance and context to understand the reason for code contributions, so all these breadcrumb trails are relevant and helpful. They'll save us time in asking each other questions that were already covered (sometimes!) and they'll help both author and reviewer to be more effective.
- If you don't understand something, don't give up and pass the buck to another reviewer. Challenge yourself: I will learn something new here. You may not be able to absorb all the context of the code change being made the first time. But if you resolve to understand just one piece of code from the pr that you didn't, then next time you'll be a more valuable reviewer. (this one is especially aspirational for me because in the past I would only deeply review things I already understood well, but remember the Automattic creed!)
- Always ask each other for tests. And if you see tests, ask "have all the cases been covered". One of our biggest weaknesses in Woo is our broadly weak and inconsistent approach to testing. We need to hold each other accountable to more broad and comprehensive testing of the features we develop.
- Please remember especially for async folks that timely review is really important to ensure we can bounce the pr back and forth between timezones, but also consider, if you're leaving the PR in an actionable state with your review. For example not giving explicit approval or requirement to make a change leaves the PR author with unclear direction. And this applies to all communications in reviews, if we don't give each other clear actionable steps we are actively delaying the author's ability to merge their feature.
- Watch your github notifications. This was something I was guilty of in the past because I didn't have them under control. It feels really frustrating as an author to come back to Slack and have to reping you about discussions being had in Github. It's not always clear if you saw the notification, forgot or are just formulating a reply. Personally I separate a portion of my day to going through my active github notifications to make sure I haven't missed any discussions or pings. In fact it also can help if you can't action a review or response right away, you can reach out to the author and let them know. Again the extra communication is just super helpful.
I'm interested to know from any other developers or reviewers too, what do you think we can do better to make the review process smoother for each other? In understanding how to do this better I think we have to listen to each other's needs and do the best we can to acommodate, because review might be one of the most important quality gates we have to delivering a better product.