4 ms·

So basically everything that one would normally finish with a PR, but without a PR. In my daily work, I never reject a PR for failing tests (the CI system disa

So basically everything that one would normally finish with a PR, but without a PR. In my daily work, I never reject a PR for failing tests (the CI system disallows merging until all tests, lint, warnings, danger, style have passed). So whenever I find a problem in a PR, it's due to something that is not detected (or possibly not even detectable) by the automated checks. By dropping the PR requirement, you lose that added check. FWIW in a team of only experienced (>10 years) devs, I find problems in approximately 20% of the PRs I review (granted our codebase has numerous subtleties due to the esoteric nature of our domain). We've also had problems that even the reviewer missed; problems that broke mainline and made merging impossible until the issues were fixed. It's fine to say "we'll just accept the added risk", but you really need to do it with eyes open. I personally won't accept such risks unless I absolutely have to (i.e. can't find a reviewer and the customer is breathing down our necks). That's one thing that 25+ years in the industry has taught me.

twic 3y ago
> By dropping the PR requirement, you lose that added check. About a quarter of the article is about how to do code reviews in the context of trunk-based development.
- kstenerud 3y ago
  Yes, but they are unconvincing. Pair programming is only acceptable to some developers (others it drives insane). It also induces tunnel vision because now both developers are too close to the code to see issues that an outside observer will pick up quickly. They're also intimately familiar with the structure and algorithms, and so they won't see things that someone fresh would have trouble grasping (meaning that they need to either simplify, name with intent, or comment appropriately). I actually haven't noticed too much of a difference in issues picked up on pair programmed vs solo programmed PRs; they tend to match the quality of the stronger developer. I've done periodic code reviews at three separate companies. Unless I've been very unlucky, it generally goes like this: * Everyone gets pulled away from their work for an hour and goes into a room. * Someone has picked some code to review (either they've been forced into it, or they have something cool they want to show off). Mostly they're just terrified at the prospect of being humiliated in front of everyone. * Everyone pretends to be interested, but they're really just waiting for it all to end as the CO2 levels slowly climb. * A few minor details are talked about, and then someone asks why approach Y wasn't used. Relieved that there's finally a discussion point, everyone talks about this architectural idea for the rest of the hour. * Due to time constraints, 90% of the code is never reviewed. I spend about an hour a day on average reviewing PRs, and that's with a team of 5 engineers. If we reviewed all code in team meetings, that would multiply review time by 5x because now everyone is reviewing all code in series rather than in parallel. Then there's this: > Pipeline approvals: If your team uses a Continuous Delivery pipeline to deliver changes to production, you can include a stage that requires someone to authorize the change to progress. This is conceptually similar to a pull request in that it is a gate in the delivery process, but you place the gate after code integration and automated tests. Doing this means that a human only spends time reviewing code that has already been proven technically correct. Except it hasn't been proven technically correct. It's just code that has been (maybe) pair programmed, and (maybe) discussed if it happened to be part of the 10% of the code looked at and discussed for architecture in the review meeting.

twic 3y ago

> By dropping the PR requirement, you lose that added check. About a quarter of the article is about how to do code reviews in the context of trunk-based development.

kstenerud 3y ago
Yes, but they are unconvincing. Pair programming is only acceptable to some developers (others it drives insane). It also induces tunnel vision because now both developers are too close to the code to see issues that an outside observer will pick up quickly. They're also intimately familiar with the structure and algorithms, and so they won't see things that someone fresh would have trouble grasping (meaning that they need to either simplify, name with intent, or comment appropriately). I actually haven't noticed too much of a difference in issues picked up on pair programmed vs solo programmed PRs; they tend to match the quality of the stronger developer. I've done periodic code reviews at three separate companies. Unless I've been very unlucky, it generally goes like this: * Everyone gets pulled away from their work for an hour and goes into a room. * Someone has picked some code to review (either they've been forced into it, or they have something cool they want to show off). Mostly they're just terrified at the prospect of being humiliated in front of everyone. * Everyone pretends to be interested, but they're really just waiting for it all to end as the CO2 levels slowly climb. * A few minor details are talked about, and then someone asks why approach Y wasn't used. Relieved that there's finally a discussion point, everyone talks about this architectural idea for the rest of the hour. * Due to time constraints, 90% of the code is never reviewed. I spend about an hour a day on average reviewing PRs, and that's with a team of 5 engineers. If we reviewed all code in team meetings, that would multiply review time by 5x because now everyone is reviewing all code in series rather than in parallel. Then there's this: > Pipeline approvals: If your team uses a Continuous Delivery pipeline to deliver changes to production, you can include a stage that requires someone to authorize the change to progress. This is conceptually similar to a pull request in that it is a gate in the delivery process, but you place the gate after code integration and automated tests. Doing this means that a human only spends time reviewing code that has already been proven technically correct. Except it hasn't been proven technically correct. It's just code that has been (maybe) pair programmed, and (maybe) discussed if it happened to be part of the 10% of the code looked at and discussed for architecture in the review meeting.