alt Hacker NewsI woke up one day and the industry is gating all code changes behind PRs w/ approvals. Anyone still able to commit to main without approval from someone else? I’m even seeing companies require multiple approvals on every PR. It’s insanity.
I’m skeptical. Software is as buggy as it ever was. I come across teams shipping terrible quality software, where every line change was approved and reviewed. I come across teams that require every change have an approval, but don’t require 100% test coverage. I’m seeing senior engineers have to get approval from juniors for a copy change.
It’s theatre. It’s bad management. It’s a cargo cult. It isn’t actually driving code quality.
Code review is one thing. Code review is good. But requiring every change have an approval is something else.
Replies
There's two kinds of reviews in my experience:
1. Does it work? Then ship it. This is great for early on, high-velocity where the goal is to get something working in the wild. AI and AI proponents love this option. It's easy to spot obvious problems, but very unlikely to lead to feedback on structural changes to abstractions and architecture to increase overall _long-term_ velocity.
2. We assume this works, but is it "correct"? This is where long-term code maintainability is created. The quality and effort put into a review like this is obviously far more involved than option 1. People working long term on a code base love this option.
We've been biased towards #1 for a long time, but I feel like we dont have enough people capable of doing #2.
exactly, the kind of reviews which have a point are the ones you do before starting work. But that's a design review, not a code review. My team does "commit to master", although I did catch a fair few regressions by looking at the committed code.... but as long as it doesn't go live, who cares?
Where I work I can and am still allowed to push to mainline directly, as are all other developers on the team.
Some new colleagues started with PRs and since then it has been a slow move towards using PRs more and more (but still not mandatory).
As of now some colleagues don't typically do PRs and push directly (minority), some decide on a case-by-case basis (I am among those, among a small majority in our team) and some have been using PRs for each and every change from day one (also a minority).
The criticisms by the opponents of PRs are as follows:
1. People relying on PRs too much causes them to propose code which is not production quality as of the PR whereas before (i.e. "no PRs") if your code was not correct, you or someone else either noticed shortly after (checking the tests on CI etc.) or it would make it into the release. I don't believe this to be generally true but sometimes thought similar when reading some PRs. The frequency of large bugs being found by code review in the PR has gone down in recent times which leads me to believe that the colleagues have adjusted their development style to come up with good quality solutions in the first attempt already in most cases.
2. Code review is hard to do in many cases. In our team this is typically resolved by going over a PR together for the “hard cases” (e.g. in a videoconference).
I think PRs are mostly worth the effort because any bug that can be avoided before the release saves a lot of downstream effort (e.g. support) and the effect of building a shared knowledge about the code page is valuable (although hard to quantify).