4 ms·

Yeah, that's a fun way to get massive stacks of PRs that are individually incomprehensible.

t-writescode 2mo ago
How? If one PR builds off another, won’t either: PR 1 is size 400 PR 2 is size 400 + 400 new PR 3 is size 800 + 400 new If they’re truly disjoint, would it be so bad to get them as unique? Because otherwise, when PRs depend on each other, you tend to get “one and then one and then one”. At least that’s how it’s worked on teams I’ve worked on that have soft size limits.
- eek2121 2mo ago
  Size is an issue, but it isn't just about size. Ideally, agile development builds linearly in complexity. Rather than dumping a huge new feature, first introduce the building blocks and the reason you are introducing them, then the glue that ties them together, then the actual feature. From what I've seen (not in software dev anymore, however I've been in it for close to 30 years), AI just tends to pile everything in, and it is very hard to review. No public model performs even average under the rules I've mentioned. Also, simply breaking up a PR doesn't count if instead you dump all the PRs on maintainers at once. Humans are the bottleneck here, and can only review so much at once. If i were still involved in PR reviews, it doesn't matter if you gave me a single 4,000 line PR or 4 1,000 line PRs, I"d reject them. What I want to see. Small, easily reviewable features with a build up to the main course, along with a good explanation for each. After that? I'd probably still reject it for a breach of code standards, or documentation, or because I don't like you sending me a PR at 4:59pm on a Friday. ;) Humans also can't blindly rely on AI for review, so the models (more precisely, the folks building the underlying stuff) must adapt.
wiml 2mo ago
Then reject them for being incomprehensible? Look, if you don't think code review is worthwhile, don't do it. Just give everybody unfettered permission to merge. But don't pretend to do review if you're not trying to maintain some standard of quality.
- striking 2mo ago
  I do think code review is worthwhile, not sure how you read that from my comment. A cap on PR size isn't inherently going to make an LLM do a good job of segmenting PRs. It requires careful prompting or manual action, the kind of effort typically exerted by people who already cared enough not to hit such a cap. You may as well just ditch the cap, to save yourself from having to reject a series of PRs rather than just the one.
  - paimapi 2mo ago
    is there a reason why there's no standardization in orgs in terms of skills/harnesses/etc for AI-assisted development? for example, a rule of 'you must invoke ABC skill that contains all of the context for this part of the codebase if you plan on making changes there' with the codeowning team dedicated to maintaining it both for their own use and for the use of other teams that have up or downstream dependencies
  - Arainach 2mo ago
    This is defeatism. Reject unacceptable PRs, full stop. It's on the author to work to break them up.
IshKebab 2mo ago
Luckily GitHub supports stacked PRs now! So they have to at least pass CI individually.

How? If one PR builds off another, won’t either: PR 1 is size 400 PR 2 is size 400 + 400 new PR 3 is size 800 + 400 new If they’re truly disjoint, would it be so bad to get them as unique? Because otherwise, when PRs depend on each other, you tend to get “one and then one and then one”. At least that’s how it’s worked on teams I’ve worked on that have soft size limits.

eek2121 2mo ago
Size is an issue, but it isn't just about size. Ideally, agile development builds linearly in complexity. Rather than dumping a huge new feature, first introduce the building blocks and the reason you are introducing them, then the glue that ties them together, then the actual feature. From what I've seen (not in software dev anymore, however I've been in it for close to 30 years), AI just tends to pile everything in, and it is very hard to review. No public model performs even average under the rules I've mentioned. Also, simply breaking up a PR doesn't count if instead you dump all the PRs on maintainers at once. Humans are the bottleneck here, and can only review so much at once. If i were still involved in PR reviews, it doesn't matter if you gave me a single 4,000 line PR or 4 1,000 line PRs, I"d reject them. What I want to see. Small, easily reviewable features with a build up to the main course, along with a good explanation for each. After that? I'd probably still reject it for a breach of code standards, or documentation, or because I don't like you sending me a PR at 4:59pm on a Friday. ;) Humans also can't blindly rely on AI for review, so the models (more precisely, the folks building the underlying stuff) must adapt.

wiml 2mo ago

Then reject them for being incomprehensible? Look, if you don't think code review is worthwhile, don't do it. Just give everybody unfettered permission to merge. But don't pretend to do review if you're not trying to maintain some standard of quality.

striking 2mo ago
I do think code review is worthwhile, not sure how you read that from my comment. A cap on PR size isn't inherently going to make an LLM do a good job of segmenting PRs. It requires careful prompting or manual action, the kind of effort typically exerted by people who already cared enough not to hit such a cap. You may as well just ditch the cap, to save yourself from having to reject a series of PRs rather than just the one.
- paimapi 2mo ago
  is there a reason why there's no standardization in orgs in terms of skills/harnesses/etc for AI-assisted development? for example, a rule of 'you must invoke ABC skill that contains all of the context for this part of the codebase if you plan on making changes there' with the codeowning team dedicated to maintaining it both for their own use and for the use of other teams that have up or downstream dependencies
- Arainach 2mo ago
  This is defeatism. Reject unacceptable PRs, full stop. It's on the author to work to break them up.

IshKebab 2mo ago

Luckily GitHub supports stacked PRs now! So they have to at least pass CI individually.