I've had it on my list for a long time to write up a document on how we approach code review. As we've added more maintainers, I think it would be helpful to provide a bit more standardization between us and share best practices we've each developed. One of my main goals would be to make it less intimidating to participate in review so that more people try it.
Let's spend this time generating some content for that document.
Hi! I have a conflict that prevents me from joining this call on time, but I hope I can contribute a little bit to this discussion.
Here are a few things that worked well in my previous experience developing a code review process:
Design documents can be really useful for substantial changes, long before they get to the code review stage. The existing PR template has a few of the elements of a design document—though, for a big change, it feels incomplete and cramped, whereas for a quick fix, it feels a bit tedious. I wonder if there's a way we could incorporate design documents for larger changes, while streamlining the PR process for smaller ones. A typical design document template might include:
Goal/problem
Alternatives considered
Usage examples (for an API or developer-facing change)
User-visible changes, UI rationale
Testing plan
Security impact
Maintenance impact
Providing guidance to the reviewer can save time, and make starting the review less intimidating. The PR author could say:
Where's the best place to start reviewing? (e.g. the core class or core interface that expresses the concepts you need to understand the rest of the PR)
What are the most interesting implementation decisions that were made? (e.g. the obvious choice might have been A, but I did B for the following reasons...)
What are the highest-risk aspects of this change? (both as it affects review—what code deserves careful scrutiny; and affects testing—what needs careful testing to ensure it didn't break)
A reviewer, conversely, can then focus on areas of risk, or on identifying areas of risk or potential confusion and looking to see that they are well covered by tests and documentation.