We’ve had a very recent uptick in engineers submitting PRs of hundreds of lines across multiple files, for Jira tickets that only asked for a one-line change. The engineers involved have been using AI assistants for nearly two years now, but there seems to have been a change in the last month or so in how aggressive the new models are at changing code.
Almost as if they’re paid by the token…
Use ponytail to keep 50 line changes to 1 line, and use rtk to save tokens.
Also use an assistant to checkout someone else’s diff and review it in chunks
2 years and they still don’t know how to prompt…
I honestly wish for a PR this size. One of the ones that came across this week was 813 commits, +17K -2K.
Of the 250 commits that GitHub was willing to show it had 35 other PRs merged into this massive one. Why they thought one giant PR was somehow better I’ll never know.
Of course…high priority, please review and merge immediately. Like guys it’s gonna take me a week to make sense of this.
> 1k lines = LGTM
> 1k lines in internal tool = idc, do what you want.
> 1k lines in critical path = lol no.
Who cares? It’s company code. They want AI, they get AI 🤷
I get the blame and I have not lined a new remote job yet. So there.
Lol nothing bigger than 250 lines of code goes through at our company without complaints.
i’d say it’s a balance… you’re totally right that individual requests for review should be relatively small (mostly so that they can all fit in your head at once), but imo equally valid is that everything in main should be a complete feature/fix: if you were to be gone immediately after merging, would someone need to continue or revert the change? would there be unused code laying around?
this is where merge trains and a decent UI around them comes in handy: your main work is on a branch many small PRs each reviewed individually merge into that branch, and then when you’re done pretty much just automated integration tests, lint, and you’re good to merge the whole
but equally some people prefer to solve this with things like gitflow, or just not at all and accept that main is always in flux
reflector to support a new feature is also tricky: does it belong with the feature because it’s unnecessary abstraction without it? or is it its own PR because it stands in its own? and if it’s its own PR then how do you base your own feature branch on it before someone reviews and merges? how do you know you’re done without finishing? what if your assumptions are wrong and you need to try something new - just a lot of unnecessary churn and review?
dev is messy and as always LOC is a pretty useless metric… keeping things understandable is key, and somethings a 17k line PR is the cleanest way to proceed
The review process is all wrong if something like this is ever on the table as a single PR*.
Big changes like this were made before, and knowing how to split the work (or at least trying to work it out) used to be part of the job.
Hopefully, strong unions and worker involvement can remedy this, given we change our work culture to be closer to what projects like SQLITE and FFMPEG have (noting, of course, the fact these are FOSS, and made by volunteers, yet are very dependable), slower and stable development cycle that prioritizes high quality work that people can actually depend on and trust.
- As in one single PR you’re expected to read, instead of one backed by tests and the like.
If LLM can make big PR, LLM can split PRs
This is a joke I’m not college educated enough to understand.
It’s a summary of code changes. The green + indicates how many lines were added, the red - how many were removed (though changed or moved lines are counted twice, once as removal for the old and once as addition for the new).
The OP is supposedly being asked to read and review thousands of lines of new code written by claude. Depending on the language and claude’s writing style, those lines may be very dense and hard to read, but even if they aren’t, reading code you haven’t written is always more difficult than reading your own.
It’s basically saying “insert one ass loads of code in one go, all written by AI”.
That’s an automatic reject from me, that’s not a patch, it’s an overhaul
No
You ask your LLM of choice to look it over, completing the shit-cycle
I fully expect this to become the new normal being pushed by management.
“We identified PR reviews to be blocking our newfound AI-powered efficiency, so we are now mandating all the reviews to done by AI. Also we figured all the developers are now useless since all you do is ask Claude to solve tickets, so you are all fired”
I wonder how long it takes for the first high profile disaster happening because of a policy like that.
This is literally how corpo I work for wants us to work. They call it… Outcome baded review. But no bugs on prod lol.
How is bun doing btw after their “We used 60 agents and 200k in tokens to rewrite in Rust” ?
Actually I think it’s doing well? The language to language rewrite is actually a strong suit of LLMs, as long as there is extensive years worth of tests to check rewrite behaviours.
I don’t have practical use cases for that strong suit though.
Oh, and bun seems to be dead now with 2.5k open PRs.
And merge to main takes over an hour.
And the total rewrite cost was significantly higher than the headline (multiple Prs by Anthropic employees, rough count 20% of total LoC of rewrite)
And there is still no release in sight on Github - but Claude is shipping with rust bun afaik.
It was higher sure, but anthropic also has some of the most expensive models out there. That cost could be 5x-10x less just by going with cheaper model providers (if one were to pay the API costs, not the case for bun).
bun 1.4 was released 3 weeks ago, btw
🛑 Changes requested
Too big. Break into smaller individual PRs.
Repeat after me: “Rejected. Reason: too large of a change for one PR.”
If you can’t be bothered to write your own code, I can’t be bothered to review
The problem with Claude is that it doesn’t write code to be modular & reusable. Every tiny change requires a complete rewrite.
I’ve completely banned any code that can’t be explained. I’ve had my CTO send me code at 3 AM to implement and when I ask him what I’m looking at he just says it doesn’t need review, just push it.
Uhh, no sir, I’m not doing shit because you’ve handed me GCC and we’re MSVC.
After I bitched endlessly to the CEO about that he said I have final say on what goes into the project.
I’ve had my CTO send me code at 3 AM
I hope you don’t even respond until your next normal working hours!
I love my job, even when I have to deal with nonsense like that and I’m compensated very well to be on call 24/7.
No amount of money would make me put my health at risk like that. Been there at a job before where I was always working. No thanks.
that sets a really bad example. you shouldn’t do that to yourself and you shouldn’t allow it to happen to anyone else.
He knows best what’s best for him. If he’s explicitly paid extra to be on call 24/7, and he’s happy with that extra. Let him be om-call 24/7.
There are situations and jobs where 24/7 availability is needed. Someone has to do it. And if that someone believes he’s getting enough of a compensation for it, there’s nothing wrong with it.
well, that depends entirely on your prompt/process, it can be done
Just more prompting will do + make no mistake
unironically, a better prompt does yield better results - shocking, I know
Unironically Claude (or whatever you use) will almost always deliver code that is shit, it’s just less shit if you prompt it better, LLMs are good to make short snippets if you get stuck; And remember to fucking check what the code is and rewrite bad shit
With enough seniour developer’s time and dedication you can spend days and enough water to flood a town, so you can badly maybe do something that a junior dev can do already (your shit will still be worse). If that’s not an achievement of a modern technology I don’t know what is.
It can but I’m not looking to make things even more complicated. We have enough unexplained non-sense in the project as is, having to link it correctly is just endless pain that I don’t want to deal with.
I struggle to review a 1k line change. When people give me such big changes I normally don’t believe they’ve reviewed them either.
That’s because they haven’t.
Mystery solved!
Try working on a codebase that’s all event-driven hexagonal CQRS with hand-crafted SQL for persistence. Add additional buzzwordy methodologies to taste.
Adding a single property to your product means you now have to update an aggregate class, several DTOs, and several event classes and handlers before you can even think about touching the UI.
And that’s in your main solution. There’s also at least one facade service you’ll need to make compatible and you also need to update the event simulator used for testing. The latter night involve having to touch every single line in a 2000 lines long SQL script.
Having to go though three separate 600-2000 LOC PRs for one PBI isn’t that exotic.
forgive them lord, for they know not what they do
deleted by creator
Occasionally I do that…but only because 500 of those lines are my comments explaining everything.
Don’t worry half of those will be useless code comments
// Here I'm not using that other thing that is now completely irrelevant, but I'll leave a comment to the non-existing thing anyway because I'm avoiding it.REEEEEEEEEEEEEEEEEEEEEEEEEEEEEEE
The comment:
# This code does exactly what you asked: Never change state, only fetch the state and return the difference
the code: hallucinated database table drops
It is not even peer review anymore, unless we are pretending that claude is our peer.
LGTM
Let’s Go Topple the Monarchy!
Let’s Gamble, Try Merging is my favourite
And people were already annoyed when I had ~90 changes due to refactoring and fixing imports …
