feedReset Pull Request signoff
14 February, 2020This is a guest post generated by Chip’s twitter thread
How to subtly degrade your code quality with too much review overhead, a thread: In a big project every code change should be reviewed by another person, right? It improves quality, security, etc! Except sometimes it doesn’t, and it actively hurts your codebase…
Our team has recently adopted automated processes on top of our pull requests where any change to a PR reverts the “signed off” status of every reviewer. It is already having a net-negative impact on our quality!
To be clear we always had signoff requirements. Just, before, you could leave a couple comments on a PR, sign off, and walk away. The expectation was that if you had some minor style/spelling/etc comments the dev could fix them if they wanted, update the PR, and be done The idea here was the dev didn’t have to address your comments. It gave you the ability to say “this is good enough but could be better,” and them the ability to say “I disagree or can’t be bothered.” PRs with serious issues could always be blocked no problem if you wanted.
Now what we see instead is folks leaving comments but hitting approve and devs actively discouraged from addressing comments post-approval because it means another round of approvals, CI/CD, etc. So lots of little things start going un-addressed….
This means that naming is a little worse, there are more spelling errors or unclear language in docs / comments, flow is a little harder to understand, etc. This is not only immediately negative but has a long-term corrosive effect…
If you’ve been in lots of codebases you start to get a feel for the quality of a codebase and a lot of developers subconsciously internalize this and use it to guide their own level of effort when making changes. Good code inspires you to meet its quality. It’s a fun challenge!
On the other hand codebase with a “bad smell” is going to get less love and thoughtfulness. What started out in your codebase as little papercuts will, over time, turn it into a dumpster fire of a codebase that nobody wants to put effort in to because it is already visibly bad!
And all of this to satisfy what wasn’t a helpful requirement in the first place, that code be rigorously reviewed. There is nothing here that achieves this goal and, in fact, it leads to reviewer fatigue and less rigorous reviews all around. It’s seriously harmful.