Talk:Guidelines for a healthy code review culture

Tim says...

Don't want to lose it to the ether, so copying Tim Starling's Mastodon post here:

Bad code review thoughts:

• That's not how I would have done it.
• This code is depressingly consistent with existing code, I wish it were in a new and fancy style.

Good code review thoughts:

• What problem is this trying to solve? Does it solve the problem?
• Will this break production?
• Is it better than how it was?

Legoktm (talk) 01:14, 27 May 2023 (UTC)

Code review productivity

I talked with Karolin about serial reviewing, and she encouraged me to write it up for this page.

Serial reviewing is what I call it when a reviewer raises some issues, and then waits for a response, and then raises some more issues which were there all along. A good review should be complete, identifying every issue which must be addressed prior to a merge. It should say that it's complete (for example, "looks good except for the inline comments"), so that everyone knows that when the issues are addressed, the patch is ready. After a new patchset, only the new changes need to be reviewed, we don't need to go back and look at everything again. If the reviewer later discovers a serious functional issue that they missed at first, and is obliged to raise it at an improper time, they should give their apologies.

I think this is good practice for two reasons: first, it improves productivity, reducing the amount of time it takes for a patch to get merged. It reduces time spent waiting for reviews and responses. It reduces context switching. It reduces the need for manual testing. By having the requirements up front, it reduces the need for the same code to be designed and rewritten multiple times.

The second reason is that it gives the developer the sense that their work is valued and will be merged if certain conditions are met. This is especially important for volunteer developers, whose patches often languish for years, unmerged. There is no incentive for them to respond to a review if their code will not be merged anyway. By stretching out the review process, and concentrating on finding fault rather than finding a path to a merge, serial reviews give the developer the sense that their code will never be good enough. They may start to think that their work is being rejected for a hidden reason, unrelated to the quality of the code.

I propose to incorporate this point into the section "thoughtful efficiency".

After reading that section, there are some other changes I would make to it:

• Move "automate" to the bottom since it's not really in the theme of the guide and is rarely relevant.
• Below "Aim for clarity", add a new dot point:
  • Complete reviews: Review the entire patch and raise every issue at the earliest opportunity. When a new patchset is uploaded, review only the new changes. Aim to merge the code in the fewest number of review/response cycles.
• Keep patches small: I feel like this is not really code review advice, at least in the way the title is phrased. I've said to a lot of colleagues that I don't mind reviewing big patches, and I push back when reviewers ask me to split my patches up. It can take a full day of work to split up a big patch, and I'm definitely not saving a day of review time by doing that, I'm just making the reviewer feel a bit more comfortable and less daunted. I always want to understand the context and rationale for a patch — I want to know if it's solving the problem, not just whether it makes sense line by line. It's certainly true that reviewers should avoid asking for unrelated work. I would like to instead write something like:
  • Stay focused: A patch should have one idea and its consequences. If you see something in nearby code that you don't like, either as a reviewer or developer, make a note or file a task, don't add more changes to the commit.
• "but provide helpful review": I feel this dot point is adequately covered by my proposed changes above, and can be deleted.
• Avoid nitpicking: This section needs a definition of nitpicking. The procedure is patronising and not especially helpful. Tooling is almost never relevant. I would rewrite it as follows:
  • Avoid nitpicking:
    • It is acceptable for new code to be in a style that is consistent with existing, nearby code. If you don't like the way things are normally done, fix it in bulk in a separate commit, or raise the issue in Phabricator.
    • Frame your comments as helpful tips, not faults to be rectified. Minor style issues which do not block the merge should be marked "resolved" in Gerrit.
    • Two developers, given the same problem, will rarely write the same code. Respect creative differences. Don't repeat the work of the developer by asking them to write the exact code you would have written. The code just needs to be acceptable, it doesn't need to be perfect.

-- Tim Starling (talk) 04:30, 3 July 2023 (UTC)

@Tim Starling (WMF) Anne and I just talked through the suggested changes and added them in, thanks again for the suggestions: https://www.mediawiki.org/w/index.php?title=Guidelines_for_a_healthy_code_review_culture&diff=6023589&oldid=5913683 KSiebert (WMF) (talk) 14:26, 12 July 2023 (UTC)

@Tim Starling (WMF) Thanks so much for the feedback and suggestions, this is awesome. As @KSiebert (WMF) said, we went through and updated the page in the following ways:

• Moved the "automate" section to the bottom and added some specifics to clarify that line a bit
• Added the "complete reviews" line. I also added a line below it that incorporated some of the previous "...but provide helpful review" line, detailing what to do if you feel you cannot provide a complete review for some reason. We see this on my team at times when a patch is large/complex and find that discussing synchronously first helps enable more thorough code review.
• Replaced the "keep patches small" line with the one you suggested, although I retained one line at the end about big picture/architectural discussions
• Updated the nitpicking section to add a definition (please feel free to suggest something different!) and incorporate your suggestions. The last bullet point you suggested here is fantastic, thanks for writing it.

I have (perhaps unsuccessfully) tried to keep this page as concise as possible, but I think there are some topics that deserve more detailed writeups, including what you've written about "serial reviewing" and this idea of synchronous code review (or at least further discussion/education that happens before code review begins). I wonder if we should write separate pages on these topics and link to them from this page. ATomasevich (WMF) (talk) 14:30, 12 July 2023 (UTC)