Comment by dimbletimbers
21 hours ago
A defense of human code review I wish I saw more often, especially in light of the concerns people have about cognitive/comprehension debt: comprehension redundancy. At the end, if taken seriously, at least two people understand how the feature works (even if that number is, on average, trending closer to between one and zero). Ideally at least one of the two also comes away with a better understanding of the wider system and how the feature fits into or stands out from that landscape.
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.
11 replies →
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.
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.
7 replies →
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.
1 reply →
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
[dead]