Stop sending me huge PRs; a rant(getsmall.xyz) |
Stop sending me huge PRs; a rant(getsmall.xyz) |
This is all in addition to the fact that the models are generally poor at storytelling, because that requires a theory of mind of the person you're communicating with. Authoring for review is storytelling, it's making changes in such a way as to build confidence in the reviewer. I believe current LLMs are still years away from this.
In my opinion, if you can't do these things, you're just cosplaying software engineering. Vibe coding has its uses, as does LLM programming, I do a lot of this! But we're kidding ourselves and dropping our standards dangerously low if we think that this is software engineering.
It's probably not quite how I would approach doing git commits, but they're at least logical boundaries, and make narrative sense for a reviewer.
So, just like you said, rewrite the whole thing, THEN break it apart into bite size chunks that tell the story and feed it to others with acceptable and reasonable context.
It’s a skill that engineers need, and it pays dividends to all on the team, including you, when your coworkers ALSO start doing this back to you and you’re asked to review it.
And this is often how I proceed when working with an LLM as well. We build the whole thing, and if I think it needs to be split apart, then in another session I work with the LLM to break it apart into reasonable chunks.
This is knowledge that goes back to the beginning of software development - "Plan to throw [version] one away".
I think this could potentially become a good practice. LLMs make it so easy and cheap to just get it working and build that v1. Then you can play around with it and see if works and read the code about what could be better. Throw away the LLM generated version and now this is the part where human expertise comes in. Based on what you've learned from the v1, now guide the LLM more closely about how to write the thing and help guide it so that making small PRs that are easily reviewable and understandable are the output.
However, I have yet to see how this will play out with upstream contribs:
https://github.com/moment/luxon/discussions/1796
https://github.com/leeoniya/luxon/tree/leeoniya/perf-patches...
This seems to be the crux of the issue.
I'm guessing the most of the time, the answer is "because that's a mandatory gate to getting these changes into production". If the PR author doesn't see the value in review, it's going to be hard to convince them to write reviewable PRs.
If they're actually looking for human feedback, telling them how to submit PRs in a way that's amenable to human feedback is going to be a lot more successful.
Personally, I would leave. But that’s not always an option for everyone.
100% agree. While you are at it, consider naming and writing your functions in such a way that doesn't require a wall of comments. Clean Code uncle Bob style.
I’ve done it myself on:
* engine definitions for complex workflows and DSLs
* heavy graph theory sections that included ASCII diagrams to clarify flow.
But those functions are probably 1 in 100 or rarer. Basically everything else is good enough with basic IDE-helping javadoc style comments at best, maybe with some input parameter clarification and business logic-clarifying 1-2 line comments sprinkled throughout.
100%
but also "no" is a two letter word and one of the most important and hardest parts of being a maintainer.
Edit - apologies I misunderstood which side the ai agent should be on.
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.
PR 1 is size 400
PR 2 is size 400 + 400 new
PR 3 is size 800 + 400 new
At least that’s how it’s worked on teams I’ve worked on that have soft size limits.
At work, I've gotten into fights about PR approvals. If they are beyond us humans to review, screw it, remove the approver requirement and if CI passes, merge it.
Even worse: they can write tests that make incorrect behavior part of your spec.
Tests matter.
Writing tests can be hard, boring, tedious. But if anything should still be written by hand in the age of LLMs it’s the tests. If you’re not looking at the application code anymore, you should at least be going over the tests with a fine toothed comb.
I think they were asked before AI and even they were not easier to write.
Its same as with commits. Usually when implementing a new feature I'm just in flow, so I don't think how to properly separate changes to different commits.
I mean - not always, but usually maintaining git history in a beautiful and clean manner was extra work even before AI.
My flow state is for editing files. Once that's done and I've got something that work. It's always easy to convert those into sensible commits. Do not that the logs is not the like of "write database schema * add the index page * add the details page * add the new object form". They're more like "show the list of objects * allow object creation * show the details of a specific object". Those breaks to create the commits are more natural to the general flow state.
But the first thing I still check is consecutive comments and that goes very far as a signal whether the person sending it even tried to grok it or not
Anyway, absolutely none of that had anything to do with LLMs -- it was a function of a person who liked to control other people as much as possible. With LLMs I find they positively like to attack problems in small pieces. I can't recall ever having to ask one to subdivide the work. They usually just do that anyway.
For OSS, my suggestion is to accept issues and specs do the implementation yourself. Warp.dev has a decent model of this in Github: https://github.com/warpdotdev/warp/blob/master/CONTRIBUTING....
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.
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.
Except, I won't be spinning in my chair, I'll be out of a job. At least until cost skyrockets and outages get much worse.