Wow someone who finally has this same unpopular opinion as I do. I'm a huge fan of review-optional PRs. Let it be up to the author to make that call and if it were really important to enforce it would be more foolproof to do so with automation.
Unfortunately every time I've proposed this it's received like it's sacrilegious but nobody could tell me why PR reviews are really necessary to be required.
The most ironic part is that I once caught a production-breaking bug in a PR while at FAANG and the author pushed back. Ultimately I decided it wasn't worth the argument and just let it go through. Unsurprisingly, it broke production but we fixed it very quickly after we were all finally aligned that it was actually a problem.
To catch stupid mistakes like an extra file, an accidental debug flag, a missing compiler hint that has to be added to migration scripts etc.
To ensure someone who doesn't quite understand the difference between dev and production build pipelines doesn't break it.
To ensure a certain direction is being followed when numerous contractors are working on the code. For example a vague consistency in API designs, API param names, ordering, etc.
To check obvious misunderstandings by juniors and new hires.
To nix architect astronauts before their 'elegant' solution for saving a string to a database in 500 lines gets added.
To check the code is actually trying to solve the ticket instead of a wrong interpretation of the ticket.
To get introduced to parts of the codebase you haven't worked on much.
But as with anything you get from it what you put in.
None of those are good reasons why PR reviews are necessary. They are examples of things that it's theoretically possible a PR review might catch. But there's no information there about how likely those things are to be caught.
Without that absolutely critical information, no cost benefit analysis is possible.
In my experience across many companies, PR reviews almost never catch any of those bugs or bring any of those benefits.
If you worked with me it would be worth doing mandatory PRs.
One trick, and I'm not being sarcastic, is to read every line. Even if you don't understand the change as a whole that catches things.
Another trick, and again not sarcastic this is genuine advice. Read every line of your own PRs before you submit them. It's surprising how much I catch doing that. Same with git commits. It's also noticeable which of your colleagues don't do this as their PRs are the ones with obvious mistakes.
All of this is much easier and more effective with multiple PRs per day, and breaking bigger tickets into smaller one.
If you're constantly doing big, multi-day commits you're doing development wrong. You lose a lot of the benefits of source control and the tooling around it.
I still do big commits sometimes, especially when refactoring, but even then it's very important to keep those big commits focused on a particular change. If I find bugs or tweak a feature I've been meaning to change, I try and make a new branch and push that to main and then rebase my feature branch off main. Or cherry pick them out before the refactor PR.
> Read every line of your own PRs before you submit them
I do. Everyone should. I’m also a fan of small focused PRs.
> If you worked with me it would be worth doing mandatory PRs.
Given the number of PRs I’ve merged and the number of mistakes that reviewers have caught, I think it’s very unlikely that you’d catch those things at a frequency high enough to justify the cost.
I’m not doubting that you can find those things or that you have found them. But again I have merged thousands of PRs with hundreds of reviewers across numerous companies in multiple languages and I have never had a reviewer catch a major bug.
That’s a large enough sample size with no effect at all, that I’m going to need hard evidence to make me believe that people are finding these things at a high enough rate to justify the cost.
>Unfortunately every time I've proposed this it's received like it's sacrilegious but nobody could tell me why PR reviews are really necessary to be required.
Unfortunately every time I've proposed this it's received like it's sacrilegious but nobody could tell me why PR reviews are really necessary to be required.
The most ironic part is that I once caught a production-breaking bug in a PR while at FAANG and the author pushed back. Ultimately I decided it wasn't worth the argument and just let it go through. Unsurprisingly, it broke production but we fixed it very quickly after we were all finally aligned that it was actually a problem.