Document PR review process

[jdhoffa]

A (slightly more than-we-need) example provided by @mbarlow12 :
https://github.com/phetsims/phet-info/blob/main/checklists/code-review-checklist.md

[AlexAxthelm]

similarly: https://devguide.ropensci.org/softwarereview_reviewer.html

[cjyetman]

Is a "document review" and/or a "code review" the same as a "PR review", or something else?

[jdhoffa]

Updated title for clarity.

This is about "documenting a good PR review process"

[mbarlow12]

As a little context, a PR in stitch for adding CONTRIBUTING.md got me thinking about some of the goals for the PR review process. I found a tension between a few different competing priorities:

• ensure sound development/engineering practices (code style, architecture, testing, docs, etc...)
• minimize reviewer overhead and time
• allow for reasonable velocity for delivering features/fixes

That review checklist is an example of a way to reduce a certain degree of cognitive effort by standardizing and segmenting the review process. You didn't have to think about how to conduct a PR, you followed the template. Adding in the misc checks helped reduce noise to free the review to focus on the intrinsically harder things like implementation details or (more rarely) application architecture.

[cjyetman]

Ironically, if PRs are minimized, focused, and appropriately documented, I feel like all three of those priorities can be met, i.e. I don't think they're necessarily in conflict with each other.

[jdhoffa]

Ironically, if PRs are minimized, focused, and appropriately documented, I feel like all three of those priorities can be met, i.e. I don't think they're necessarily in conflict with each other.

Yeah, I whole-heartedly agree with that. I'd love if the team were collectively opening 1-2 right-sized PRs per day (when actively working on stuff). Small, focused, incremental self-contained improvements. The benefits are many.

FWIW we already mention that here, but it might be good to emphasize the topic more: https://rmi.github.io/practices/process/pull_request.html