Two Kinds of Code Review

Ive read a book about management and it helped me to solve a long-standing personal conundrum about the code review process. The book is High Output Management. Naturally, I recommend it (and read this review as well:

One of the smaller ideas of the book is that of the managerial meddling. If my manager micro-manages me and always tells me what to do, Ill grow accustomed to that and wont be able to contribute without close supervision. This is a facet of a more general Task-Relevant Maturity framework. Irrespective of the overall level of seniority, a person has some expertise level for each specific task. The optimal quantity and quality of supervisors involvement depends on this level (TRM). When TRM grows, the management style should go from structured control to supervision to nudges and consultations. I dont need a ton of support when writing Rust, I can benefit a lot from a thorough review when coding in Julia and I certainly require hand-holding when attempting to write Spanish! But the overarching goal is to improve my TRM, as that directly improves my productivity and frees up my supervisors time. The problem with meddling is not excessive control (it might be appropriate in low-TRM situations), it is that meddling removes the motivation to learn to take the wheel yourself.

Now, how on earth all this managerial gibberish relates to the pull request review? I now believe that there are two largely orthogonal (and even conflicting) goals to a review process.

One goal of a review process is good code. The review ensures that each change improves the overall quality of a code base. Without continuous betterment any code under change reverts to the default architecture: a ball of goo.

Another goal of a review is good coders. The review is a perfect mentorship opportunity, it is a way to increase contributors TRM. This is vital for community-driven open-source projects.

I personally always felt that the review process I use falls quite short of the proper level of quality. Which didnt really square with me bootstrapping a couple of successful open source projects. Now I think that I just happen to optimize for the peoples aspect of the review process, while most guides (with a notable exception of Optimistic Merging) focus on code aspects.

Now, (let me stress this point), I do not claim that the second goal is inherently better (though it sounds nicer). Its just that in the context of both IntelliJ Rust and rust-analyzer (green-field projects with massive scope, big uncertainties and limited payed-for hours) growing the community of contributors and maintainers was more important than maintaining perfectly clean code.

Reviews for quality are hard and time consuming. I personally cant really review the code looking at the diff, I can give only superficial comments. To understand the code, most of the time I need to fetch it locally and to try to implement the change myself in a different way. To make a meaningful suggestion, I need to implement and run it on my machine (and the first two attempts wont fly). Hence, a proper review for me takes roughly the same time as the implementation itself. Taking into account the fact that there are many more contributors than maintainers, this is an instant game over for reviews for quality.

Luckily, folks submitting PRs generally have medium/high TRM. They were able to introduce themselves to the codebase, find an issue to work on and come up with a working code without me! So, instead of scrutinizing away every last bit of diffs imperfection, my goal is to promote the contributor to an autonomous maintainer status. This is mostly just a matter of trust. I dont read every line of code, as I trust the author of the PR to handle ifs and whiles well enough (this is the major time saver). I trust that people address my comments and let them merge their own PRs (bors d+). I trust that people can review others code, and share commit access (r+) liberally.

What new contributors dont have and what I do talk about in reviews is the understanding of project-specific architecture and values. These are best demonstrated on specific issues with the diff. But the focus isnt the improvement of a specific change, the focus is teaching the author of (hopefully) subsequent changes. I liberally digress into discussing general code philosophy issues. As disseminating this knowledge 1-1 is not very efficient, I also try to document it. Rather than writing a PR comment, I put the text into or and link that instead. I also try to do only a small fixed number of review rounds. Roughly, the PR is merged after two round-trips, not when theres nothing left to improve.

All this definitely produces warm fuzzy feelings, but what about code quality? Gating PRs on quality is one, but not the only one, way to maintain clean code. The approach I use instead is continuous reafactoring / asynchronous reviews. One of the (documented) values in rust-analyzer is that anyone is allowed and encouraged to refactor all the code, old and new.

Instead of blocking the PR, I merge it and then refactor the code in a follow-up (ccing the original author), when I touch this area next time. This gives me a much better context than a diff view, as I can edit the code in-place and run the tests. I also dont waste time transforming the change I have in mind to a PR comment (the motivation bits go directly into comment/commit message). Its also easy to do unrelated drive-by fixes!

I wish this asynchronous review workflow was better supported by tools. By default, changes are merged by the author, but the PR also goes to a review queue. Later, the reviewer looks at the merged code in the main branch. Any suggestions are submitted as a new PR, with the original author set as a reviewer. (The in-editor reviewing reminds me iron workflow.)

For conclusion, let me reference another book. I like item 32 from C++ Coding Standards: be clear what kind of class youre writing. A value type is not an interface is not a base class. All three are classes, but each needs a unique set of rules.

When doing/receiving a code review, understand the context and purpose. If this is a homework assignment, you want to share knowledge. In a critical crypto library, you need perfect code. And for a young open source project, you aim to get a co-maintainer!