If only one person knows the code, then PR time isn't going to save you.
I'm tech lead and I basically don't review PRs, and I tell people this, with a caveat - if you can tell me what you specifically want me to review, for what specific purpose, I'm happy to!
So "can you review this bit for race conditions" is great, love it. This forces people to actually think about what in their code they should be suspicious of, if anything.
"Can you review this" [link to PR] is getting a rubber stamp because humans have never been good enough at "just spotting bugs" to make this worth it and now any LLM is better than a human.
And for the purpose of understanding - PR time is too late. I have not reviewed a PR ("for real") in a long time and yet I could tell you how every system my people have built works down to a very fine level of detail. And it's because _we talk to each other!_ We don't just chill in the same slack channel and code independently, we all value each others brains and want each others inputs because we know it will improve our product and we value what perspectives others will bring.
Trying to learn via PR is a sad substitute for real collaboration and teamwork.
This is wild to read, I always review PRs and frequently find bugs or significant problems in them that get them bounced back
That's a strong signal that your team isn't doing well. Significant problems should have been spotted at a software design stage, or raised in standups, or identified in a pairing session. The earlier you can find an issue the simpler it is to fix, so waiting until the last moment (e.g. PR) means you're spending far more time fixing issues than necessary.
As for finding bugs, what happens if you miss them? Do they go out to production and potentially lose user data? Finding bugs in PR is a big problem. For a start it shows your automated tests aren't good enough, and secondly it shows the devs aren't checking their code works well enough.
If you do them at all, PRs should be a gate for checking whether the code meets the team's quality bar, not if it even works. The team should be able to deliver working code without them.
I'm guessing you work on some highly technical domain that receives highly structured data and is amenable to TDD.
Not all domains are like that. The majority of bugs I see are business/domain logic bugs. It's akin to having misunderstood or missed some aspect of the question, not causing data loss.
I'm guessing you work on some highly technical domain that receives highly structured data and is amenable to TDD.
I lead a group of teams that build frontend software, so not really. :)
The majority of bugs I see are business/domain logic bugs. It's akin to having misunderstood or missed some aspect of the question, not causing data loss.
Financial loss, reputational harm, degraded UX, etc. They're all significant problems. They're more recoverable than a data loss, but equally bad from an accepted low quality standpoint.
I'm also going to guess that you don't have BAs, PMs, or people responsible for the logic reviewing the code in a PR. Consequently you can't spot those problems in at the PR gate unless the issue is that the dev didn't understand the requirements and wrote code that didn't do what it's supposed to. In which case we're back to the quality and testing problem. By raising questions in standup ("Can I clarify that I understand the AC right?"), pair programming ("Let's check the code against the AC") and communicating properly ("Can you demo the feature to the BA so we can be sure it's correct") you move the problem to the people who can answer, and stop the devs needing to review that someone wrote working code.
I just don't believe PRs are the right point to be finding out that the requirements were wrong or that the dev didn't understand what to build. That needs to happen as early as possible. PR is as late as possible.
It sounds like you work at a company where the responsibilities of engineering are split across at least three separate roles, things move slowly and in a structured way, there's a large amount of coordination work, and a PR is a methodical translation of some step-by-step process. Perhaps you have bi-weekly meetings to review RFCs or similar.
At a much smaller company, you might find that a single person does part of the job of a BA, PM, and engineer, that they can produce a PR much more quickly as a result, and that it's more common for a PR to prompt the first detailed discussion about how something will work. A small team has quicker turnaround time on PRs and design, and can thus position in-depth reviews later in the process because less work will be thrown away in the case of a rejection.
I've held this view as a senior dev in a small company, a senior dev in a big compant, the co-founder CTO of a startup with 5 devs, and I continue to hold it now I'm an EM with many teams in a really big company. It's not about team size. It's about fixing the right problem in the right place. PRs just aren't that. They're useful if you have a problem with people not meeting a good standard, but they're not useful as a gate for whether or not the code works or if it's been architected in a sensible way. You need to know those things earlier (especially in a startup where speed is paramount.)
I disagree in some cases. Code is a means of communication. Sometimes it's more efficient to write code and bring it to your team than to discuss it in the abstract.
An example of this might be adding load shedding. You could spend hours talking through it, or you could say "I'm going to add a load shedder to the blah service as a proof of concept" in standup then take an hour to implement it, and have the team critique it from there.
I agree that whether they're a useful gate is debateble, but they can be a useful means of expressing an idea to be approved or rejected.
In my teams work like that would ideally be done as a proof of concept on a branch that's ultimately throw away. It would never reach a PR. Reality doesn't always work that way, and sometimes those POCs make their way into production, but it should. I certainly wouldn't want the decision to merge it into the main codebase to be done in a PR. That sort of thing needs proper discussion.
> The earlier you can find an issue the simpler it is to fix, so waiting until the last moment (e.g. PR) means you're spending far more time fixing issues than necessary.
Wait, does that mean that E2E tests that catch bugs are a strong signal that the team isn't doing well? How about component tests that catch bugs? Wouldn't they be better caught at the unit-test level? Do you see how your argument is flawed? - nobody is "waiting until PR to catch all bugs" but that doesn't mean that PR review can't/ shouldn't catch bugs! Sometimes even significant ones, yes.
You don't eliminate E2E tests because "you have good unit tests". You shouldn't just eliminate PR reviews because "we communicate inside the team".
does that mean that E2E tests that catch bugs are a strong signal that the team isn't doing well?
Yes. E2E tests are there to give you confidence that future changes haven't broken things. They're not there to catch bugs before the feature goes to production. Unit and integration tests should do that though.
You shouldn't just eliminate PR reviews because "we communicate inside the team".
You should eliminate them as soon as they're not giving you any real value, but if you don't eliminate them before that team's will stop trying to get that value in a better way because they believe the PR process catches bugs. It doesn't though, so all it really achieves is stopping the team trying to improve.
Its a strong signal that developers make mistakes, code review has empirically been shown to be one of the best ways of uncovering defects. If you aren't finding bugs that people have written during code review (or you're not doing code review at all like the person I was replying to!), they're slipping rightwards
Bugs usually include business logic edge cases, and significant problems include someone realising while reading the PR that we can actually create a much better solution to the underlying problem. Ideally that realisation would happen prior to the PR being offered, but that's also not really how humans work
Example: During a PR review for a graphical feature for a game, someone reading it realises that we can actually have a significantly better solution to the underlying problem. You can't catch this in testing, because it doesn't even make sense conceptually to test it. It also sucks that it happened after someone put in a lot of work, but with graphics development you expect a lot of what you write to get canned and replaced with a better solution, because the technology evolves over time. The work is iterative towards the final goal anyway
Its also very common to miss subtle edge cases with graphics hardware, eg someone misunderstood the intricacies of GPU hardware, or a team member has relevant experience that someone else does not have. Or they missed a problematic memory access pattern on some hardware for example. Or something simple like they've technically forgotten a barrier, that the validation layer doesn't report for some reason
Eg: The function "tanh" is broken on some AMD GPU hardware, and should never be used under any circumstances. The actual GPU implementation of it is just screwed. Ideally everyone would know this, but its a very common function to crop up during specific graphics algorithms (as it smoothly remaps the range [-inf, +inf] -> [-1, 1]). So occasionally I've spotted that, and had to explain that we need to use an approximation instead, and then now everyone knows . It rarely gets caught during testing setups, because people don't know they need to include that hardware in their tests in the first place
> That's a strong signal that your team isn't doing well. Significant problems should have been spotted at a software design stage, or raised in standups, or identified in a pairing session.
This is a strong signal that you're looking at different things than a lot of people doing code reviews are.
- Why are you using this api for this instead of this other one? We use the other one because it avoids a specific issue.
- Your code isn't following the same patterns that we use in these places. It should be using the same patterns so that it's more obvious to anyone else that works on it
- The name you gave this function/class/whatever doesn't accurately represent what it is for
These are the kinds of things that are generally noticeable during a code review, and can make a big difference later on. And they're generally not going to be identified during standups and/or design.
Pairing is an option, since it is (effectively) code review _while_ writing the code (with a certain amount of blinders on, so not quite as effective). That being said, I hate pairing, so it's certainly not on my recommendation list.
Mhhh but it isn't how open source works, right? I only contributed once but I couldn't imagine it without starting with a PR and needing to bounce it back and forth.
How would you restructure the approach to open source? Honest question.
I consider PRs to be primarily a defense of the architecture, and to a lesser degree a general sanity check. It’s also a useful opportunity to enforce automations are being run
I consider 99% of my "defense of the architecture" strategy to be teaching my team why the architecture is important, how to think about it, and invite they commentary on it as we own and evolve it together. And of course if they are doing something and want input or are uncertain, then my door is open.
If PRs are a notable part of my architecture defense, I'm going to work on investing in the team instead of reviewing PRs.
> I consider 99% of my "defense of the architecture" strategy to be teaching my team why the architecture is important, how to think about it, and invite they commentary on it as we own and evolve it together.
Nice way to avoid the responsibility for any team fuckup: it's not me, I only teach them, they decide themselves.
>Nice way to avoid the responsibility
Welcome to the tech industry. Its all about shifting responsibility in case something goes wrong. Thats the only reason companies use third party software in the first place, to have a scapegoat...
performance reviews are also a great time to find a scapegoat as well
"Teaching" doesn't work well for plenty of people. I couldn't count the number of presentations, design review meetings, tech talks, etc. I've attended that I remember nothing from. On the other hand, putting something into code, getting feedback and understanding how something affects the system I care about - that's something that sticks.
Ok, so you ignore PRs and don’t catch people who made architectural mistakes until what, they go to production?
I mean, do that too, but end of the day PRs are your final chance to catch the mistakes before they start cementing, and is your best opportunity to identify misunderstandings (you can smell the confusion in their changes and address it directly).
But also, I must defend against the hordes of unwashed masses and maintain the sanctity of my domain. End of the day, a codebase I own is a codebase I own, and others cannot be allowed to poison the well, intentionally or not. That’s how you get cholera
Very well put. A PR is the final (and ultimate) chance to say "no" to a change - after that it's part of the software, it needs to be maintained, it can have impact on stability/quality etc.
I don't quite understand how someone in charge of a software (techlead or similar title) can't take this serious; I know a few such people and I generally prefer to avoid working with them...
Just going to nitpick on one thing:
> "Can you review this" [link to PR] is getting a rubber stamp because humans have never been good enough at "just spotting bugs" to make this worth it and now any LLM is better than a human.
Spotting bugs is the one thing we do have evidence that code inspection is good for. But there's a massive difference between the type of code review there's good evidence for and a github-style PR review, so it's mixed but not entirely without foundation.
PRs are the final chance to avoid mishaps; be it some junior overdoing DRY, be it someone in the team misunderstanding (or insufficiently understanding) a requirement, something being forgotten, edge-case missed - practically anything! Before/During PR, a single person owns the code, after merge it's everyone in the project.
Someone who just rubberstamps PRs works either in a completely different setting than anything I can imagine or it's just someone who doesn't take ownership & responsibility as serious as I'd require people I want to work with; I can't quite see much room for gray area there...
THANK YOU. I’ve always felt this way about PR review. I feel like people should write a natural language description of the change, and every section of it should link to part of the diff, and every part of the diff should be linked to by part of the description. That or just leave a comment on every chunk of the diff.
You might have use for an issue tracker I've been building the past year and a half. It lets you inspect diffs inline in the ticket, and replay the board to see how the workflow evolved over time via a timeline scrubber.
https://ljtn.github.io/epiq/
It stores issues as an immutable event log in your repo, so you can go back and inspect the context behind a change without having to litter the code with comments. Helps with traceability of intent.
A proper natural language description of the change without links to parts of the diffs is already advanced material.
And arguably if the description needs linking to parts of the diffs then the commit is too large?
This sounds like a fussier version of what Donald Knuth was doing with literate programming.
Which, incidentally, is a really enjoyable way to work.
People in my team are generating long and verbose PR descriptions with AI. No idea why and for whom. I certainly never read any of them.
Man I'm so sick of seeing 50-line PRs with 800 lines of AI slop markdown files included. I also don't read them.
I follow mailing lists (emacs and openbsd) and sending a diff is kinda the boundary between wishing for something and making the something into a thing. It’s the difference between discussing a plot and discussinf a draft.
Sneding a PR should not be for understanding or just for rubber stamping. It’s about getting someone to look at your approach and helping you find flaws or proposing ideas that could make it better.
When I review PR, the primary question is: For the stated problem, is the diff a good solution? Sometimes I don’t know enough about the problem, so I just try to see if the code has glaring mistakes (mispellings, styles,…) but those are just comments, not suggestions.
Well articulated. Ty