The golden rule of PR reviews
You’re reviewing Jane’s pull request.
It fixes a bug that stops many customers using your software.
Unfortunately, her variable names look strange.
Do you “Approve” her PR, or do you “Request changes”?
Your choice here is between two product variants:
|
Product A |
Product B |
| Usability |
❌ Some customers can't use it |
🎉 Customers can use it |
| Variable names |
😐 No changes |
🤔 Some questionable names |
If you had to choose between Product A and Product B,
which would you choose?
The bugfix clearly outweighs the variable name choices.
Therefore, you should approve Jane’s pull request.
This is The Golden Rule of PR Review:
If It’s An Improvement, Approve It.
With the decision framed this way,
no reasonable person would choose Product A.
And yet in my experience, many reviewers would not approve Jane’s PR!
This is usually because they’re not following the Golden Rule,
but instead following some other incorrect rule.
Let’s see some examples.
John hit “Request changes” on this PR, writing:
“I would have written this loop functionally. You can use a map function here.”
In doing so, John was implicitly answering the question:
“Is this a change I would have made?”
But this is the wrong question:
you’re different people, and there’s more than one way to do it!
Perhaps John was influenced by the possibility that he might be blamed for approving Jane’s review.
(And perhaps John was influenced by the unfortunate name “code review”,
which wrongly emphasizes “code” as the thing that matters.)
As a response to John’s review policy,
Jane will instead request review from team members
that she believes would have implemented the change similarly.
Alex hit “Request changes”, writing:
“All PRs should maintain or improve coverage. Please add unit tests before merging.”
In doing so, Alex was implicitly answering the question:
“Is every aspect of this PR an improvement?”
But this too is a wrong question:
almost everything is an engineering trade-off!
Perhaps Alex was influenced by all those CI metrics (coverage, code size, bundle size, build time),
which wrongly emphasize aspects that are easily measurable,
and which imply that there is no trade-off amongst them.
As a response to Alex’s review policy,
Jane will not bother submitting future improvements,
because the cost of submitting a change is too high.
Bob hit “Request changes”, writing:
“The page looks bad on mobile. Please use a responsive design.”
In doing so, Bob is implicitly answering the question:
“Is this the ideal implementation?”
But this is yet another wrong question:
product development is iterative.
Perhaps Bob was influenced by Gitflow,
an antiquated style in which
“developers create a feature branch and delay merging it to the main trunk branch until the feature is complete.”
As a response to Bob’s review policy,
changes will pile up on long-running “feature” branches,
leading to slower software delivery.
Sometimes people complain about reviewers being “petty tyrants”,
drunk on the small amount of power that this task gives them.
I don’t think this is the main reason for overly strict reviews.
Rather, I think the primary cause is that reviewers are not taught the Golden Rule:
If It’s An Improvement, Approve It.
Similar posts
More by Jim
What does the dot do in JavaScript?
foo.bar, foo.bar(), or foo.bar = baz - what do they mean? A deep dive into prototypical inheritance and getters/setters. 2020-11-01
Smear phishing: a new Android vulnerability
Trick Android to display an SMS as coming from any contact. Convincing phishing vuln, but still unpatched. 2020-08-06
A probabilistic pub quiz for nerds
A “true or false” quiz where you respond with your confidence level, and the optimal strategy is to report your true belief. 2020-04-26
Time is running out to catch COVID-19
Simulation shows it’s rational to deliberately infect yourself with COVID-19 early on to get treatment, but after healthcare capacity is exceeded, it’s better to avoid infection. Includes interactive parameters and visualizations. 2020-03-14
The inception bar: a new phishing method
A new phishing technique that displays a fake URL bar in Chrome for mobile. A key innovation is the “scroll jail” that traps the user in a fake browser. 2019-04-27
The hacker hype cycle
I got started with simple web development, but because enamored with increasingly esoteric programming concepts, leading to a “trough of hipster technologies” before returning to more productive work. 2019-03-23
Project C-43: the lost origins of asymmetric crypto
Bob invents asymmetric cryptography by playing loud white noise to obscure Alice’s message, which he can cancel out but an eavesdropper cannot. This idea, published in 1944 by Walter Koenig Jr., is the forgotten origin of asymmetric crypto. 2019-02-16
How Hacker News stays interesting
Hacker News buried my post on conspiracy theories in my family due to overheated discussion, not censorship. Moderation keeps the site focused on interesting technical content. 2019-01-26
My parents are Flat-Earthers
For decades, my parents have been working up to Flat-Earther beliefs. From Egyptology to Jehovah’s Witnesses to theories that human built the Moon billions of years in the future. Surprisingly, it doesn’t affect their successful lives very much. For me, it’s a fun family pastime. 2019-01-20
The dots do matter: how to scam a Gmail user
Gmail’s “dots don’t matter” feature lets scammers create an account on, say, Netflix, with your email address but different dots. Results in convincing phishing emails. 2018-04-07
The sorry state of OpenSSL usability
OpenSSL’s inadequate documentation, confusing key formats, and deprecated interfaces make it difficult to use, despite its importance. 2017-12-02
I hate telephones
I hate telephones. Some rational reasons: lack of authentication, no spam filtering, forced synchronous communication. But also just a visceral fear. 2017-11-08
The Three Ts of Time, Thought and Typing: measuring cost on the web
Businesses often tout “free” services, but the real costs come in terms of time, thought, and typing required from users. Reducing these “Three Ts” is key to improving sign-up flows and increasing conversions. 2017-10-26
Granddad died today
Granddad died. The unspoken practice of death-by-dehydration in the NHS. The Liverpool Care Pathway. Assisted dying in the UK. The importance of planning in end-of-life care. 2017-05-19
How do I call a program in C, setting up standard pipes?
A C function to create a new process, set up its standard input/output/error pipes, and return a struct containing the process ID and pipe file descriptors. 2017-02-17
Your syntax highlighter is wrong
Syntax highlighters make value judgments about code. Most highlighters judge that comments are cruft, and try to hide them. Most diff viewers judge that code deletions are bad. 2014-05-11
This page copyright James Fisher 2023. Content is not associated with my employer. Found an error? Edit this page.
TigYog
Kickabout