aaaaalbert commented 5 years ago

Following discussions with @awwad and @lukpueh, I'd like to suggest a new practice for code reviews: Code authors should voice their expectations of a code review, and reviewers should state the performed actions and areas/topics of focus.

This proposal is motivated by the observation that code reviews are difficult to perform to a degree that exhausts the code quality space -- style, logic, documentation, testing, deployment, etc. They in turn tend to get shelved, which stalls progress. It's natural that not everyone is an expert in everone else's topic, so I think it's good to be explicit about the depth and dimensions of code reviews.

However, this proposal is not meant to duplicate the existing developer workflow documentation. It rather serves to augment that document's last step, "Request a review".

At a high level

The necessary prerequisite is that code authors make sure that the changes they propose actually make sense and work at a basic level. (Corollary: It should never happen that neither the author nor the reviewer actually tests the fix.)

When submitting a PR for a working patch, the author may request specific actions or areas of focus for the code review:

"Please do / look for / look at the following: ...."

The reviewer complements this by meta-annotating their review, i.e. supplementing the actual review items with an overall statement:

"Here's what I did to review your code: ...."

Step by step

From the code author's side:

Step 0: Make sure your proposed change actually works.

Step 0, part 2: This also means you make sure it conforms to the code style etc. right away. (Do not waste the reviewer's time with things you can easily get right in the first place!)

Step 1: Send a PR and list what you think are the aspects that require the largest share of a reviewer's attention.

Step 2: Feel free to suggest the review to someone, and/or ping them/the group to remind them of it.

From the reviewer's side:

Step 0: Inform the world that you are working on this.

Step 1: Perform the review.

Step 2: Mention the areas (topical, regional) you focus on. Feel free to mention areas you cannot review as well.

Examples

I think it's good to come up with a list of examples of items/actions that authors could request and reviewers could provide.

I'm happy to go over my list of review comments to start this. Perhaps you have things in mind right away, so let's just post items below. If it makes sense, the list can be incorporated into PR templates at some point.

JustinCappos commented 5 years ago

I like this. It seems very clear and clearly valuable! Would you be willing to reformat this as a markdown file and send this as a PR that integrates it into our site?

lukpueh commented 5 years ago

Fixed by #18