6 ms·

After 20 years of doing this, I’m convinced that required PR reviews aren’t worth the cost. In the thousands of pull requests I’ve merged across many companies

After 20 years of doing this, I’m convinced that required PR reviews aren’t worth the cost. In the thousands of pull requests I’ve merged across many companies, I have never once had a reviewer catch a major bug (a bug that is severe enough that if discovered after hours, would require an oncall engineer to push a hot fix rather than wait for the normal deployment process to fix it). I’ve pushed a few major bugs to production, but I’ve never had a PR reviewer catch one. I’ve had reviewers make excellent suggestions, but it’s almost never anything that really matters. Certainly not worth all the time I’ve spent on the process. That being said, I’m certainly not against collaboration, but I think required PR reviews aren’t the way to do it.

kevmo314 2y ago
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.
- sarchertech 2y ago
  >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. Obvious signs of cargoculting in my opinion.
  - deleted 2y ago
    [deleted]
- mattmanser 2y ago
  I'll bite. 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.
  - sarchertech 2y ago
    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.
    - mattmanser 2y ago
      Well, I catch those things. Now you have some information. 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.
dullcrisp 2y ago
The point of code reviews isn’t to catch bugs. It’s for someone else on the team to read your code and make sure they can understand it. If no one else on your team can understand your code, you shouldn’t be committing it to the repository.
- deeviant 2y ago
  I feel if you ask 5 people what "the point" of codes review is, you'd get 6 different answers.
  - deleted 2y ago
    [deleted]
  - zimpenfish 2y ago
    And a 7th complaining about the formatting of the question.
- klabb3 2y ago
  HN moment. I’ve never seen in practice that someone says ”I don’t understand it” and the author says ”good point, I will simplify it”. Rather, the opposite. I often saw people make unnecessary complex or large PRs that were too much workload to review, leading the reviewer to approve, on the grounds of ”seems like you know what you’re doing and tbh I don’t have half a day to review this properly”. Code review is a ritual. If you ask why we have it people will give you hypothetical answers more often than concrete examples. Personally I’m a proponent of opt-in CRs, ie ask for a second pair of eyes when your spidey senses tell you.
  - Capricorn2481 2y ago
    > I often saw people make unnecessary complex or large PRs that were too much workload to review, leading the reviewer to approve, on the grounds of ”seems like you know what you’re doing and tbh I don’t have half a day to review this properly” That just seems like company wide apathy to me. Obviously you have to make an effort to read the code, but there are lots of ways developers can overcomplicate things because they were excited to try a pattern or clever solution. It doesn't make them bad devs, it's just an easy trap to fall into. These should not pass a code review just because the code "works." It's totally acceptable to say "we're not gonna understand this in 3 months the way it's written, we need to make this simpler" and give some suggestions. And usually (if you're working with people that care about the workload they make for others) they will stop after a few reviews that point this out. We've done this at our company and it's helped us immensely. Recognizing whether the code is unnecessarily complex or the problem is inherently complex is part of it, though.
ozim 2y ago
Unfortunately for compliance reasons PRs are required. Funny part is that not even in highly regulated markets. ISO270001 or SOC2 are pretty much something every software company will have to do.
- kevmo314 2y ago
  Curious because I am not familiar: are PRs required or are PR reviews required?
  - ozim 2y ago
    Well "Peer Review" or "Code Review" is required - pull requests are easiest way to have it all documented with current state of art tooling. Otherwise you have to come up with some other way to document that for purpose of the audit.
- __turbobrew__ 2y ago
  Yes, this is why we have required PR reviews at my company. It is to meet compliance controls. We recently talked about not requiring reviews for people in L5 and above levels but ultimately got shut down due to compliance.
- sarchertech 2y ago
  SOC2 doesn't require code reviews. SOC2 is just a certification that you are following your own internal controls. There's nothing that says required PR reviews have to be one of your internal controls. That's just a common control that companies use.
  - ozim 2y ago
    I would argue that "common control that companies use" falls under "industry standard" and I would say it would make it harder to pass certification without PR reviews documented on GitHub or something alike. So it does not require but everyone expects you to do so :)
    - 2y ago
tdrz 2y ago
The fact that there is a PR review process in place, makes commiters try harder. And that's good!
- gizzlon 2y ago
  Or try less because they have to spend time doing pr reviews
  - queuep 2y ago
    Yes, same for QA sometimes.. dev sets bar lower as the QA can test it. Just makes a bunch of back and forth. And when stuff breaks nobody feels responsible.
sneak 2y ago
Required PR reviews means that if someone steals your credentials, or kidnaps your child, you can't get something into production that steals all the money without someone else somewhere else having to push a button also. It's the two-person rule, the two nuclear keyswitches.
- sarchertech 2y ago
  This is definitely not why PR reviews are required. Most companies don't really know why they require them, but I've definitely never heard one say it was because they were afraid of malicious code from stolen credentials. There's so many other ways you can inject malicious code with stolen credentials that doesn't require a PR in every production environment I've ever worked in. There's much lower hanging fruit that leaves far fewer footprints.
zimpenfish 2y ago
> I have never once had a reviewer catch a major bug Just in 2024, I've had three or four caught[0] (and caught a couple myself on the project I have to PR review myself because no-one else understands/wants to touch that system.) I've also caught a couple that would have required a hotfix[1] without being a five-alarm alert "things are down". [0] including some subtle concurrency bugs [1] e.g. reporting systems for moderation and support
JKCalhoun 2y ago
I agree with you. If you give each dev a kind of sand-box to "own" within a project they'll learn to find their own bugs, write both simple and robust code, lots of param checking — grow as an engineer that way.
jasonlotito 2y ago
Allowing anyone to promote anything to production without any other eyes on it is problematic. Not realizing this is extremely telling. The presumed claim that no one at the company benefited from a second set of eyes is amazing, too.
- sarchertech 2y ago
  >Allowing anyone to promote anything to production without any other eyes on it is problematic. In my experience the people who are promoting things to production that shouldn't be will find a way to do it. They'll either wear down the people who want to stop it, or they'll find someone else to approve it who doesn't know why it shouldn't be approved or doesn't care. My hypothesis is that requiring any 2nd random engineer in the company to approve production code doesn't provide enough value to justify the cost. There may be other controls that are worth the cost. However, our industry has been shipping software for a long time without this requirement, and I've seen no evidence that the practice has saved money, reduced the number of bugs, or improved software quality by any other metric. I think it's time we examine the practice instead of taking it on faith that it's a net benefit. >Not realizing this is extremely telling. Nice way of saying, I don't agree with you so I must be an idiot.
- jvans 2y ago
  but there isn't actually a second set of eyes because the second set of eyes you're thinking about is complaining about formatting or slamming the approve button without actually looking

kevmo314 2y ago

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.

sarchertech 2y ago
>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. Obvious signs of cargoculting in my opinion.
- deleted 2y ago
  [deleted]
mattmanser 2y ago
I'll bite. 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.
- sarchertech 2y ago
  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.
  - mattmanser 2y ago
    Well, I catch those things. Now you have some information. 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.

dullcrisp 2y ago

The point of code reviews isn’t to catch bugs. It’s for someone else on the team to read your code and make sure they can understand it. If no one else on your team can understand your code, you shouldn’t be committing it to the repository.

deeviant 2y ago
I feel if you ask 5 people what "the point" of codes review is, you'd get 6 different answers.
- deleted 2y ago
  [deleted]
- zimpenfish 2y ago
  And a 7th complaining about the formatting of the question.
klabb3 2y ago
HN moment. I’ve never seen in practice that someone says ”I don’t understand it” and the author says ”good point, I will simplify it”. Rather, the opposite. I often saw people make unnecessary complex or large PRs that were too much workload to review, leading the reviewer to approve, on the grounds of ”seems like you know what you’re doing and tbh I don’t have half a day to review this properly”. Code review is a ritual. If you ask why we have it people will give you hypothetical answers more often than concrete examples. Personally I’m a proponent of opt-in CRs, ie ask for a second pair of eyes when your spidey senses tell you.
- Capricorn2481 2y ago
  > I often saw people make unnecessary complex or large PRs that were too much workload to review, leading the reviewer to approve, on the grounds of ”seems like you know what you’re doing and tbh I don’t have half a day to review this properly” That just seems like company wide apathy to me. Obviously you have to make an effort to read the code, but there are lots of ways developers can overcomplicate things because they were excited to try a pattern or clever solution. It doesn't make them bad devs, it's just an easy trap to fall into. These should not pass a code review just because the code "works." It's totally acceptable to say "we're not gonna understand this in 3 months the way it's written, we need to make this simpler" and give some suggestions. And usually (if you're working with people that care about the workload they make for others) they will stop after a few reviews that point this out. We've done this at our company and it's helped us immensely. Recognizing whether the code is unnecessarily complex or the problem is inherently complex is part of it, though.

ozim 2y ago

Unfortunately for compliance reasons PRs are required. Funny part is that not even in highly regulated markets. ISO270001 or SOC2 are pretty much something every software company will have to do.

kevmo314 2y ago
Curious because I am not familiar: are PRs required or are PR reviews required?
- ozim 2y ago
  Well "Peer Review" or "Code Review" is required - pull requests are easiest way to have it all documented with current state of art tooling. Otherwise you have to come up with some other way to document that for purpose of the audit.
__turbobrew__ 2y ago
Yes, this is why we have required PR reviews at my company. It is to meet compliance controls. We recently talked about not requiring reviews for people in L5 and above levels but ultimately got shut down due to compliance.
sarchertech 2y ago
SOC2 doesn't require code reviews. SOC2 is just a certification that you are following your own internal controls. There's nothing that says required PR reviews have to be one of your internal controls. That's just a common control that companies use.
- ozim 2y ago
  I would argue that "common control that companies use" falls under "industry standard" and I would say it would make it harder to pass certification without PR reviews documented on GitHub or something alike. So it does not require but everyone expects you to do so :)
  - 2y ago

tdrz 2y ago

The fact that there is a PR review process in place, makes commiters try harder. And that's good!

gizzlon 2y ago
Or try less because they have to spend time doing pr reviews
- queuep 2y ago
  Yes, same for QA sometimes.. dev sets bar lower as the QA can test it. Just makes a bunch of back and forth. And when stuff breaks nobody feels responsible.

sneak 2y ago

Required PR reviews means that if someone steals your credentials, or kidnaps your child, you can't get something into production that steals all the money without someone else somewhere else having to push a button also. It's the two-person rule, the two nuclear keyswitches.

sarchertech 2y ago
This is definitely not why PR reviews are required. Most companies don't really know why they require them, but I've definitely never heard one say it was because they were afraid of malicious code from stolen credentials. There's so many other ways you can inject malicious code with stolen credentials that doesn't require a PR in every production environment I've ever worked in. There's much lower hanging fruit that leaves far fewer footprints.

zimpenfish 2y ago

> I have never once had a reviewer catch a major bug Just in 2024, I've had three or four caught[0] (and caught a couple myself on the project I have to PR review myself because no-one else understands/wants to touch that system.) I've also caught a couple that would have required a hotfix[1] without being a five-alarm alert "things are down". [0] including some subtle concurrency bugs [1] e.g. reporting systems for moderation and support

JKCalhoun 2y ago

I agree with you. If you give each dev a kind of sand-box to "own" within a project they'll learn to find their own bugs, write both simple and robust code, lots of param checking — grow as an engineer that way.

jasonlotito 2y ago

Allowing anyone to promote anything to production without any other eyes on it is problematic. Not realizing this is extremely telling. The presumed claim that no one at the company benefited from a second set of eyes is amazing, too.

sarchertech 2y ago
>Allowing anyone to promote anything to production without any other eyes on it is problematic. In my experience the people who are promoting things to production that shouldn't be will find a way to do it. They'll either wear down the people who want to stop it, or they'll find someone else to approve it who doesn't know why it shouldn't be approved or doesn't care. My hypothesis is that requiring any 2nd random engineer in the company to approve production code doesn't provide enough value to justify the cost. There may be other controls that are worth the cost. However, our industry has been shipping software for a long time without this requirement, and I've seen no evidence that the practice has saved money, reduced the number of bugs, or improved software quality by any other metric. I think it's time we examine the practice instead of taking it on faith that it's a net benefit. >Not realizing this is extremely telling. Nice way of saying, I don't agree with you so I must be an idiot.
jvans 2y ago
but there isn't actually a second set of eyes because the second set of eyes you're thinking about is complaining about formatting or slamming the approve button without actually looking