It is reasonable to break PRs up into smaller chunks, but there is a limit. There are frequently reviewers who get very zealous about this and insist on breaking things up beyond what is reasonable, for example if breaking it up would destroy the intent, or if the "thousand line" PR just contains lots of tests (AI's love to write tests, and I love that they do that). Some tasks are just long, and its important to contextualise this when reviewing.
In the end though, these reviewers will die off like the dinosaurs. The article actually states that they find the idea of reviewing a large PR with AI bad because "it wastes your tokens reviewing a reingesting code that was already made by an AI". This doesn't make a whole lot of sense, AI will frequently reingest AI generated content, evals are a great example of this.
Just after this the article touches on the real issue at play: "okay, great, why did you put it up for a human review at all then?". Indeed, this is a good question to ask, why do we put it up for human review? I would wager that they don't actually want human feedback, a human has placed themselves as a gatekeeper and thus must be placated, and probably chooses the most inefficient way to keep that gate slowing everyone down who has kept up with the technology of our times.
I like this comment. I also have a lot of experience with lazy (my accusation!) reviewers who ask me to split a PR into smaller PRs. At some jobs, I felt like it was a strategy to sabotage my work (slow me down).
In my experience, the best was to ensure your code will be approved (ok, maybe some minor tweaks) is to assign a code reviewer before you start writing code. You (the coder) performs some quick analysis, then formulates a plan for how to fix the bug or implement a change or new feature. You verbally discuss this plan using screenshare (or side-by-side in-person) so both of you can see the code that will be changed. The code reviewer needs to verbally approve your plan. This way, you don't spend a lot of time writing code and tests, only to have the reviewer rejected it very quickly: "You should have done it this way instead." (Please note: That process that I described is intentionally informal, casual, and unrigid. Why? This grants permission for the coder and reviewer to decide the plan as intelligent adults, not as "Children of JIRA" [hint: unnecessary formality].)
Another thing that I do: After I write the code, but before I write tests, I ask the reviewer to review, but not approve. After looking at their comments, I quickly add another commit to the PR to address their concerns, then start work on the tests. When the reviewer does the final review, it is very quick, like 10% of the initial review. All of this really helps to reduce coder/reviewer friction, and nearly eliminate "Tyranny of the Reviewer". One last gripe about code reviewing: It hardly makes sense for someone much more junior than the coder to do the review. I have seen this too many times.
Yes this. As an author I see it as my responsibility to socialise my work early and get buy in from reviewers.
The more I think about PR review, which I've been doing a lot lately thanks to AI, the more I see it as social in nature. Built on trust and reputation. It's no wonder OSS is tending to ban AI contributions - they don't have trust and reputation to fall back on as a heuristic.
> It's no wonder OSS is tending to ban AI contributions - they don't have trust and reputation to fall back on as a heuristic.
Have you heard about Linus Torvald's opinion about using AI/LLMs for Linux kernel patches? He seems quite open-minded about it. Roughly: "It is a useful tool."
Yes I think his stance works well for established contributors who have a reputation. For infrequent contributors, with no rep, it's a large liability to try to figure out if their plausible-at-a-glance work is actually slop. Such a liability with uncertain ROI hence the sentiment in OSS frequently banning or at least heavily policing AI usage
What I'm trying to get at is if a PR comes in that looks decent at a glance, well in the past that was a strong signal and now it's not. But you can fall back on the relationship you have with the author. Trust , reputation. In OSS often you have no relationship with the author (of course ex established contributors or people who have well established public reputation). Whereas in a typical workplace you will quickly establish a relationship with those you work with
why do we put it up for human review? I would wager that [...] a human has placed themselves as a gatekeeper
Well, yes, presumably because the maintainers have an obligation to their users (or whoever else they answer to), so I doubt they're overly willing to relegate themselves to being a rubber stamp for whatever some LLM hallucinates as looking good to it.
Comments
It is reasonable to break PRs up into smaller chunks, but there is a limit. There are frequently reviewers who get very zealous about this and insist on breaking things up beyond what is reasonable, for example if breaking it up would destroy the intent, or if the "thousand line" PR just contains lots of tests (AI's love to write tests, and I love that they do that). Some tasks are just long, and its important to contextualise this when reviewing.
In the end though, these reviewers will die off like the dinosaurs. The article actually states that they find the idea of reviewing a large PR with AI bad because "it wastes your tokens reviewing a reingesting code that was already made by an AI". This doesn't make a whole lot of sense, AI will frequently reingest AI generated content, evals are a great example of this.
Just after this the article touches on the real issue at play: "okay, great, why did you put it up for a human review at all then?". Indeed, this is a good question to ask, why do we put it up for human review? I would wager that they don't actually want human feedback, a human has placed themselves as a gatekeeper and thus must be placated, and probably chooses the most inefficient way to keep that gate slowing everyone down who has kept up with the technology of our times.
I like this comment. I also have a lot of experience with lazy (my accusation!) reviewers who ask me to split a PR into smaller PRs. At some jobs, I felt like it was a strategy to sabotage my work (slow me down).
In my experience, the best was to ensure your code will be approved (ok, maybe some minor tweaks) is to assign a code reviewer before you start writing code. You (the coder) performs some quick analysis, then formulates a plan for how to fix the bug or implement a change or new feature. You verbally discuss this plan using screenshare (or side-by-side in-person) so both of you can see the code that will be changed. The code reviewer needs to verbally approve your plan. This way, you don't spend a lot of time writing code and tests, only to have the reviewer rejected it very quickly: "You should have done it this way instead." (Please note: That process that I described is intentionally informal, casual, and unrigid. Why? This grants permission for the coder and reviewer to decide the plan as intelligent adults, not as "Children of JIRA" [hint: unnecessary formality].)
Another thing that I do: After I write the code, but before I write tests, I ask the reviewer to review, but not approve. After looking at their comments, I quickly add another commit to the PR to address their concerns, then start work on the tests. When the reviewer does the final review, it is very quick, like 10% of the initial review. All of this really helps to reduce coder/reviewer friction, and nearly eliminate "Tyranny of the Reviewer". One last gripe about code reviewing: It hardly makes sense for someone much more junior than the coder to do the review. I have seen this too many times.
Yes this. As an author I see it as my responsibility to socialise my work early and get buy in from reviewers.
The more I think about PR review, which I've been doing a lot lately thanks to AI, the more I see it as social in nature. Built on trust and reputation. It's no wonder OSS is tending to ban AI contributions - they don't have trust and reputation to fall back on as a heuristic.
Yes I think his stance works well for established contributors who have a reputation. For infrequent contributors, with no rep, it's a large liability to try to figure out if their plausible-at-a-glance work is actually slop. Such a liability with uncertain ROI hence the sentiment in OSS frequently banning or at least heavily policing AI usage
"There's no trust or reputation in OSS" is a pretty interesting take.
What I'm trying to get at is if a PR comes in that looks decent at a glance, well in the past that was a strong signal and now it's not. But you can fall back on the relationship you have with the author. Trust , reputation. In OSS often you have no relationship with the author (of course ex established contributors or people who have well established public reputation). Whereas in a typical workplace you will quickly establish a relationship with those you work with
Well, yes, presumably because the maintainers have an obligation to their users (or whoever else they answer to), so I doubt they're overly willing to relegate themselves to being a rubber stamp for whatever some LLM hallucinates as looking good to it.