Software design principles for teams: The Principle of Non-blocking Review Feedback.
If you must use code reviews to gate merging, support review feedback that does not block merging.
Background
This is an elaboration of one of the principles I originally published in the following article…
Principle
If you must use code reviews to gate merging, support review feedback that does not block merging.
Note: this is the most specific, targeted principle on this list. You could rightly accuse me of being petty :-). However, breaking the connection between code review and integration has enormous potential for many teams that are re-looking at their ways of working.
As I’ve aged as a software engineer, I’ve been saddened by the de facto adoption of coding practices from inherently asynchronous, low-trust environments (i.e., open source software, or OSS) into environments that shouldn’t contain those traits, namely professional software engineering. Specifically, the use of (1) branch-based development to isolate code from the trunk, (2) pull requests (PRs) as the only way to merge to trunk, and (3) policies around code review prior to merging.
At this point in writing, it is important to admit my own biases around code review:
I believe that every line of production code should undergo some form of review
My preferred approach for this review is via pair programming
Anything that stops written code from serving customer needs in production is a potential source of waste
I’ve yet to work in an environment where a culture of blocking PRs weren’t hugely wasteful and/or inefficient
No amount of onerous policy will make up for trust issues within the engineering teams, and surrounding organisations
Everything is a trade-off :-)
The OSS origin
When contributing to open source, maintainers are often looking for like-minded souls to help share the burden of upkeep, but this relationship usually exists on nothing but a social contract. Contributors to OSS are typically unknown to the maintainers and - beyond a shared interest in the current state of the codebase - there is unlikely to be alignment on the vision of the codebase. This is inherently a low-trust environment and blocking merges via PRs is appropriate to allow maintainers to ensure contributions are useful, and of acceptable quality. Blocking PRs also allow contributions to happen at a time convenient to the maintainer, useful given the global spread of people contributing to open source.
Let’s compare this environment to what we would expect to find inside a commercial software engineering team…
The commercial software alternative
The level of trust within such a team should be orders of magnitude higher than the OSS counterpoint. There are both social and commercial contracts governing the relationship. While there may not be 100% alignment on the vision of the codebase, there should be broad understanding and acceptance of the direction in which it is going. Code quality will vary based on experience and other factors, but contributors are professional software engineers with a baseline expectation of quality work. Remote work may have led to a broader span of working hours, locations and timezones, but this is still be inherently a high-trust environment. Insisting all changes to trunk wait behind a mandated set of blocking code reviews makes a mockery of this expectation, and provides a backdoor for any number of unseemly behaviors to emerge.
I would say that git normalising working with branches, and GitHub successfully popularising branch-based workflows are the biggest negative impact on developer efficiency of any of the changes I’ve seen since entering the industry in the mid 1990s.
But this horse has long since bolted, and the milk is certainly spilled, so where to from here? Blocking PRs are an enormous source of waste in many organisations and often a facade for code review theatre, but there is one area which can be changed quickly and simply because it is a tweak to the code review process itself… support non-blocking code review changes.
“Our greatest waste is not unproductive engineers but work products sitting idle in process queues.”
Don Reinertsen (“Principles of Product Development Flow”)
Non-blocking code review changes
A non-blocking code review comment on a PR looks like this: “when we’re next in this part of the codebase, let’s bring this controller in line with the others”. The key part of this is the phrase “when we’re next in this part of the codebase” because it sets the priority of this suggestion as something that doesn’t block the PR from being merged until yet another round of developer update and review.
Key points about supporting non-blocking code reviews:
The team needs to have sufficient trust that the “when we’re next in this part of the codebase” promise is satisfied. Creating smaller PRs is both a good practice in terms of flow. Creating smaller PRs is also likely to increase the chance that subsequent PRs will cover the same code. Conversely, a culture where engineers ignore this review feedback is likely to snuff out any attempt to support this change.
The team needs to have sufficient process to capture these “do later” items, especially with larger teams. Having good discipline around TODO comments is an easy way to start, given how widespread IDE support for these comments has become. Perhaps one day even the tooling that largely contributed to us getting into this mess will help support a way out :-)
Reviewers need to have a clear and shared understanding about what feedback does not need immediate attention. Mature reviewers will ideally conclude that a very, very low percentage of their comments should block code being merged. Barring bugs or security issues, I would argue that everything else can be iterated upon, and the cost/risk of delayed integration is higher.
This principle is not something I originated, just an approach I’ve come to appreciate while helping teams to move towards Trunk-based Development. Unsurprisingly, there are some good practical ways of approaching this from a number of other sources:
A description of post-integration code reviews → https://andywine.dev/non-blocking-code-reviews-github/
To see how the Netlify UX team categorises review feedback into blocking and non-blocking → https://www.netlify.com/blog/2020/03/05/feedback-ladders-how-we-encode-code-reviews-at-netlify/
For a more detailed description of the problems associated with blocking code reviews → https://www.scrum-tips.com/agile/synchronous-code-reviews/
Kent Beck’s thoughts on code review →