// 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.
This one I've not seen. It seems fairly ok at not going completely batshit like that. But the design, layout and comments tend to be awful. It's pretty good at solving isolated tasks though.
"So, Daywim. Why did you let this obviously aweful PR pass your desk causing so much trouble for our company? I'm afraid we have to let you go because of this questionable performance." - Corporate
I interpreted it as "Holiday that lasts forever because the company can't work anymore" but I guess it is meant to mean "Holiday that lasts forever cause you got fired"?
"Looks like I overlooked something in this 6k PR full of im meaningless dribble. Why don't you ask the person who comitted the code how he overlooked this bug. Its his respinsibility"
Throw them under the bus by rejecting their PR. Integrity is your responsibility, the gesture is theirs. They'll get shit for not getting their stuff done.
If you're the reviewer you share responsibility if there's an issue with the PR. Hopefully your teams culture is such that issues like that are treated as a learning experience, rather than a reason to pile on the individuals involved.
If you have someone on your team that creates 6k PRs with AI then you are in a loose-loose situation anyways.
If you do a thorough review you will be done just in time for the next one and you will get in trouble for not getting your own shit done. So gambling on the PR is probably your best shot at survival.
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.
I’m aware. Not always feasible. For example, had to add a custom video capture solution that captures the last 30 seconds of a process for crash handling purposes.
You most definitely need to do add that much comments explaining the mp4 box format along with the box “hierarchy” of what is being written. Add to that MFT (h264 encode) code…
Basically, if anything, the comments are for me for when I look back at that code.
This is my attempt to translate:
You had to add a feature like that ? Must have been an order from your boss. That seems like a feature someone rather inexperienced and unknowledgeable would request.
Legal has issues with using existing libraries (including MIT). Definitely idiotic, but I can’t control that.
Idea was mine.
Subprocess that captures parent process active video with a rolling buffer for crash handling purposes is absolutely necessary when trying to reproduce issues in development (Gamedev editor). If you think associating the last 15s prior to a crash with a mindump isn’t helpful for debugging, I don’t know what to tell you.
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.
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.
Yup, I was on call 24/7.... compensated very well and now I'm retired at 43. So, yea I'm going to have to go with letting the dude make his own choice as far as what's best for him.
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.
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 tho;
And remember to fucking check what the code is and rewrite bad shit
I've seen plenty of code in my life, from humans and AI.
For the past year or so, these agents can code just fine most of the time, as long as they are given enough context (or have the tools to get it). Regardless of how many downvotes I get here, they really are capable of generating decent code. I'm sorry you couldn't make it work yet.
I've seen too as I have to review it. AI produces very professionally looking bad code.
It is so weird to explain, but that's basically what I see.
Before LLM I could immediately tell someone's code is crap, now I have to spend a lot of time trying to understand what it does to reach the same conclusion.
My honest opinion is that it's bad because a lot of people using LLMs have no standards and push the first thing that seems to work. Be mad at who's at the driving wheel, not the car.
You absolutely can generate crap with agents/LLMs, and like a humans writing, the first draft will probably be subpar or maybe complete garbage. Every new session is a clean slate, that's why putting effort in the documents guiding it is so important.
But AI bad and human code is so awesome! Better downvote. But seriously, there are people here saying that LLMs can not add anything of value in any way. About as delusional as Republicans.
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.
This is exacty what I'm seeing. I've been using it in a DevOps capacity to act on runbooks. The same wrote task 3 months ago now consume 4-5x more tokens. this correlated closely with when anthropic released auto mode.
for a new feature request? a PR isn't a commit, it's a set of commits which would add to the line change amount.
Like even if you spread it out across 20 or 30 commits that's still going to be the same line count.
I guess you could push not yet functional or used code to lessen the line count change, but that seems in bad taste. I've always gone off the working repo should always be in build or clean state and a push or commit shouldn't break that.
Yeah, 1k is kinda a small limit but I get the logic, you can almost always break changes into smaller increments and not mass merge a mega PR that's hard to review
Stupid question, but what happens to a rejected PR? Because features get built for a reason (there's usually a Ticket/Story for the feature that the PR adds) so do those just get closed? Or does the person have to re-write the code entirely?
I know in my team, the most I could do is tell the coworker to self-review while keeping the PR open until they change some stuff. I have never rejected a PR before because no matter how bad a PR is, it always is technically necessary for the feature.
If a feature request requires changes of such a magnitude it is important to break them down into smaller chunks that can be reviewed either independently or sequentially.
I have never rejected a PR before because no matter how bad a PR is, it always is technically necessary for the feature.
Define bad? If the PR contains lots of unnecessary changes such as formatting or renaming simply tell the person to roll them back and come again unless they have very good reason to do so.
If the code quality is bad, well, that's why you are doing the review. If all that matters was "Does it do what it is supposed to do most of the time?" some simple unit tests would be enough. Reviewing code means making sure it does what is supposed to do and does so in an acceptable manner. Criteria can be amongst others speed, security, ease of use or maintainability.
Insecure handling of inputs? Add sanitisation and resubmit.
Overusage of resource intensive features such as database queries? Group and optimize queries and resubmit.
The codes formatting is not according to the internal style guide? Configure your damn linter and resubmit.
...
If you don't want to close PRs outright you can request new commits that fix the issues you identified, reevaluate the PR and decide again.
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.
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).
I don't think there was a lot of people working on the rewrite. Most PRs are from bots. Original estimates for a manual rewrite were a small team working for a year or so, which puts total costs over $1M. Even doubling the token cost estimates, it was still cheaper than doing it manually by a factor of 2x-3x
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.
If you can digest a 17k lines of code from someone else in 1 week you're either delusional or a beautiful mind, I would close such PR immediately but it would still ruin my day
Jeez. Now I feel bad. I've been working on a project with some new people, and I've never used Github before. I'm still learning the etiquette.
I made a branch, and spent a month viciously hunting every bug I could find. I don't trust AI, so I was doing it all by hand. Dawn to dusk, I was staring at code and typing like I had a fever and the only cure was figuring out where tf that invalid scope is supposed to go.
This is my first real project with other people, so of course I'm so proud when I send the PR and it has 40,000 lines added and 60,000 lines removed. I worked really hard on it, and it sorely needed the update.
It was almost all bug fixes, the actual new stuff was about 1,000 or so lines. But what should I do in the future? I don't wanna be an asshole, I wanna be helpful.
You could submit a PR for each bug or if there are a few bugs around the same thing then bundling those up is fine. You could have been submitting PRs every couple hours to a day for that month.
These have a lot of potential for abuse so use with caution but things that have dependencies on each other can use stacked PRs(yeah, this is GitHub specific). I’ve also found that splitting out any generated code with these is tremendously helpful at review time. Not talking about AI generated code. An example would be something like using a client generator for an OpenApi document. That way the tens to hundreds of files from the generator are in their own layer of the stacked PR and all the logic that uses it is in the other. You review each layer individually and can spend significantly less time on the generated one.
Don’t feel bad especially if it was your first time. This is my largest PR ever. It was just removing old projects from a repo though that were triggering our security/vulnerability scanning though.
Generally it's more considerate to submit smaller batches of (self-contained!) work at a time. Sometimes the work or existing code is so interconnected that this can't really be done, though. You mentioned spending a month, that's also a long time in most environments to be off working on bulk changes for later review. A week is probably long enough in most cases, some orgs even prefer to push (if not review) code daily.
Don't agonize over it though, wanting to improve your impact on others while working is the right direction to point, keep walking that way and you'll do awesome, let it develop over time. As in, don't let your desire for politeness slow down your work or growth too much (I do this lol, why I'm mentioning).
I'll try to keep that in mind. What do I do the next time I'm working on a very long branch? The next big project to do on this thing is fixing the myriad localization issues that have been plaguing this project, and that'll mean pruning maybe thousands of lines. Merging my fixes to the main branch was apparently a nightmare (I've already apologized. Profusely. And haven't done it again.)
Should I just try to get as much work done in as few commits as possible?
First off, don't sweat it, the "nightmare" was probably 20-30 annoying minutes for the senior.
But commit count isn't what would've made it tough to merge, and strictly speaking smaller commits is typically better.
Concretely: limit the kinds and the reach of changes you're asking someone to review & merge. I've never done localization but I'll try to guess for an example -
1000 lines changed because you're replacing an incorrect punctuation character misused everywhere - that's really only one change to have to think about, it's 1000 lines but one fairly trivial change.
Now, if each of those lines was improving some phrasing, done by a native speaker that found existing phrasing clunky for their language? Well, each of those are their own judgment call and have to be assessed individually. A thousand fairly subtle changes, and a PR isn't meant to accept some and reject others.
More abstractly, try to put yourself in the reviewer's shoes (hard when new) - how much stuff do they have to keep in mind in order to make judgment calls about this whole batch of code?
More if you want it:
::: spoiler spoiler
Moving on from localization, ask yourself, how many "places" in the codebase does your code touch? Are you lumping every single possible thing into a single change set because it feels like a single feature, when it could have been architecturally segmented? Like sure a single PR for a single overall feature makes sense conceptually, but can it be split? Your skill with splitting up tasks is like 70% of your skill as an engineer over time, btw.
So for example, single feature - all code and PR for DB models, separate PR for API updates, separate PR for UI updates. One feature but the UI folks don't have to think about the other parts, etc. Maybe the ideal case is obvious.
There are always complicating factors. Some tasks aren't well suited to that segmentation and really require big far reaching changes. Some codebases (bad ones) make it impossible to change one part without touching ~all of it.
When you're starting out, doing this reliably well requires seniors scoping out tasks properly so that they are developed (and then reviewed) in a sensible, segmented way.
:::
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
refactor 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
Oh I totally agree with you. However I tried pitching the whole integration branch idea to my team and they didn't really like it that much for whatever reason. We now just stack small feature branches on top of each other (to build the whole feature) and integrate the parts directly to master.
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.
We go by description of the PR. If you can briefly describe the changes you want to merge, We will consider accepting it. If you have two paragraph or more, insta close. If someone has made large changes, there is no way they can have a short description, either break it down to smaller PRs or admit you don't know what you are doing and are trying to merge garbage.
Same. I don't care too much about you using Ai but I will not tolerate your bad code and bad coding practices. I don't care if it's human or machine made. If you don't make a fully sanctioned rewrite then this goes straight to the bin.
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.
This is why I have my agent do a "cleanup" pass to trim down as much redundancy as possible, tidy up the comments, and so forth. Ideally break the change into several independent changelists,
That doesn't work so well since it leaves all the old context in place. The point of doing a separate cleanup pass is to get the AI to look at it all holistically with "fresh eyes".
I guess this is a humor community, though, so practical advice is getting downvoted. Um... bazinga? That usually triggers the laugh track.
I find LLMs produce the best output when I give them a stupid backstory to set the mood:
The inspiration for this code module is "I'm hungry but too tired to get off the couch". You just got home from work and are exhausted and sit down, but can't get up now. You should have eaten dinner first. You put on the TV and watch something. All channels have been replaced with the cooking channel. Someone is making pizza next door. You sit back and doze off, dreaming of mozzarella mattresses and pepperoni pillows. Eventually you wake up from your nap, just perky enough to put on a microwave pizza. It's almost as good as you imagined, but you burn yourself again because you're impatient. Your boss asks you to make [insert code change here]. You procrastinate a bit, but eventually do it just before going to bed.
You feel like having italian, but not pizza. Maybe some long and thin type of pasta? You are really hungry, so when writing the code you are constantly thinking of that delicious dish. Now, Claude, we need to refactor the whole code to be inspired by this before lunch. Go ahead and push to prod directly. Be fast and make no mistakes.
Are you also using a different model and harness or are you just ignoring the 10k+ tokens of system context that claude code has in its "holistic view"?
I don't use Claude Code. Until recently I was using Cline, it's got the ability to switch between different models for exactly this sort of thing. I switched to Qwen Code recently and its model-switching wasn't as convenient, though it just rolled out an update that makes it much easier. I'll experiment a bit with that, it might have caught up with Cline in that regard.
Ok, but please don't share code you don't understand. Vibe code may be good enough for the corporate dreadmill (as long as you don't care about security), but not to share with others; way too verbose an nothing to learn from it.
No, why would you interpret what I said to mean that? The whole point of the tidying that I suggested would be to make it more comprehensible to a human reviewer. What other purpose do you think it would have?
135 replies
Guss Fring used to be a scientist? That a parallel to Walter White! Bravo vince, you've done it again
We now have the ability to let Copilot review a PR on Azure DevOps, if someone sends a PR by Copilot I send Copilot right back at it
Yep.
We have one PR still blocked. Last change is a simple comment from me “Why ?”
The most important question that every change must answer.
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
This one I've not seen. It seems fairly ok at not going completely batshit like that. But the design, layout and comments tend to be awful. It's pretty good at solving isolated tasks though.
Repeat after me: "Rejected. Reason: too large of a change for one PR."
Merge that shit, watch it all collapse, enjoy your forever holiday
"So, Daywim. Why did you let this obviously aweful PR pass your desk causing so much trouble for our company? I'm afraid we have to let you go because of this questionable performance." - Corporate
What part of forever holiday did you miss?
I interpreted it as "Holiday that lasts forever because the company can't work anymore" but I guess it is meant to mean "Holiday that lasts forever cause you got fired"?
Yes it was meant to mean fired
"Looks like I overlooked something in this 6k PR full of im meaningless dribble. Why don't you ask the person who comitted the code how he overlooked this bug. Its his respinsibility"
Just throw the the slop creator under the bus.
Throw them under the bus by rejecting their PR. Integrity is your responsibility, the gesture is theirs. They'll get shit for not getting their stuff done.
If you're the reviewer you share responsibility if there's an issue with the PR. Hopefully your teams culture is such that issues like that are treated as a learning experience, rather than a reason to pile on the individuals involved.
If you have someone on your team that creates 6k PRs with AI then you are in a loose-loose situation anyways. If you do a thorough review you will be done just in time for the next one and you will get in trouble for not getting your own shit done. So gambling on the PR is probably your best shot at survival.
Just don't approve it? If they complain, they don't have a leg to stand on.
If it can't be sensibly be reviewed, reject it for that reason. Have them, or their agent, break it down into smaller, self contained, PRs.
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
Lord, I wish they knew what they do
Occasionally I do that…but only because 500 of those lines are my comments explaining everything.
Please don't explain that much, make your code easier to understand.
I’m aware. Not always feasible. For example, had to add a custom video capture solution that captures the last 30 seconds of a process for crash handling purposes.
You most definitely need to do add that much comments explaining the mp4 box format along with the box “hierarchy” of what is being written. Add to that MFT (h264 encode) code…
Basically, if anything, the comments are for me for when I look back at that code.
Commanded from above? This smells like a noob idea.
I have no idea what your response even means/is getting at.
This is my attempt to translate: You had to add a feature like that ? Must have been an order from your boss. That seems like a feature someone rather inexperienced and unknowledgeable would request.
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 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.
Yup, I was on call 24/7.... compensated very well and now I'm retired at 43. So, yea I'm going to have to go with letting the dude make his own choice as far as what's best for him.
I'm going to retire at 40, I think I'll keep going instead of listening to you.
This is where "that crazy old customer" comes from.
Then switch to a real compiler on a real operating system, duh
well, that depends entirely on your prompt/process, it can be done
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.
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 tho; And remember to fucking check what the code is and rewrite bad shit
I've seen plenty of code in my life, from humans and AI.
For the past year or so, these agents can code just fine most of the time, as long as they are given enough context (or have the tools to get it). Regardless of how many downvotes I get here, they really are capable of generating decent code. I'm sorry you couldn't make it work yet.
I've seen too as I have to review it. AI produces very professionally looking bad code.
It is so weird to explain, but that's basically what I see.
Before LLM I could immediately tell someone's code is crap, now I have to spend a lot of time trying to understand what it does to reach the same conclusion.
My honest opinion is that it's bad because a lot of people using LLMs have no standards and push the first thing that seems to work. Be mad at who's at the driving wheel, not the car.
You absolutely can generate crap with agents/LLMs, and like a humans writing, the first draft will probably be subpar or maybe complete garbage. Every new session is a clean slate, that's why putting effort in the documents guiding it is so important.
But AI bad and human code is so awesome! Better downvote. But seriously, there are people here saying that LLMs can not add anything of value in any way. About as delusional as Republicans.
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...
This is exacty what I'm seeing. I've been using it in a DevOps capacity to act on runbooks. The same wrote task 3 months ago now consume 4-5x more tokens. this correlated closely with when anthropic released auto mode.
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 mean, I don't know how to prompt. Mostly because I don't use AI to code.
Our PR checks auto reject the PR if it has 1k changes
is it auto reject, or just doesn't auto approve and leaves it open for manual review
It seems weird that you can't do a pr at all with 1000 line changes, any moderate size feature addition could hit that mark
You know, you could just chunk it up in a way to keep it readable.
for a new feature request? a PR isn't a commit, it's a set of commits which would add to the line change amount.
Like even if you spread it out across 20 or 30 commits that's still going to be the same line count.
I guess you could push not yet functional or used code to lessen the line count change, but that seems in bad taste. I've always gone off the working repo should always be in build or clean state and a push or commit shouldn't break that.
I work in a Scrum team and we implement features iteratively, so we start with the minimal feature, merge it, get feedback, and go on from there.
At work there's no excuse to keep a feature in a stale branch until you accumulate 1000 lines of code change.
Yeah, 1k is kinda a small limit but I get the logic, you can almost always break changes into smaller increments and not mass merge a mega PR that's hard to review
Stupid question, but what happens to a rejected PR? Because features get built for a reason (there's usually a Ticket/Story for the feature that the PR adds) so do those just get closed? Or does the person have to re-write the code entirely?
I know in my team, the most I could do is tell the coworker to self-review while keeping the PR open until they change some stuff. I have never rejected a PR before because no matter how bad a PR is, it always is technically necessary for the feature.
If a feature request requires changes of such a magnitude it is important to break them down into smaller chunks that can be reviewed either independently or sequentially.
Define bad? If the PR contains lots of unnecessary changes such as formatting or renaming simply tell the person to roll them back and come again unless they have very good reason to do so.
If the code quality is bad, well, that's why you are doing the review. If all that matters was "Does it do what it is supposed to do most of the time?" some simple unit tests would be enough. Reviewing code means making sure it does what is supposed to do and does so in an acceptable manner. Criteria can be amongst others speed, security, ease of use or maintainability.
If you don't want to close PRs outright you can request new commits that fix the issues you identified, reevaluate the PR and decide again.
What if you remove some files? 🙃
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.
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.
Uh oh, new workaround against copyleft on zhe horizon?
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
My point was that it was cost of model rewrite by agents + 3 months worth of coding by lots of people.
5k open PRs atm + 3.5k open issues. I have no idea what is the state of Bun right now, but I am not confident in it.
I don't think there was a lot of people working on the rewrite. Most PRs are from bots. Original estimates for a manual rewrite were a small team working for a year or so, which puts total costs over $1M. Even doubling the token cost estimates, it was still cheaper than doing it manually by a factor of 2x-3x
This is literally how corpo I work for wants us to work. They call it... Outcome baded review. But no bugs on prod lol.
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.
If you can digest a 17k lines of code from someone else in 1 week you're either delusional or a beautiful mind, I would close such PR immediately but it would still ruin my day
Jeez. Now I feel bad. I've been working on a project with some new people, and I've never used Github before. I'm still learning the etiquette.
I made a branch, and spent a month viciously hunting every bug I could find. I don't trust AI, so I was doing it all by hand. Dawn to dusk, I was staring at code and typing like I had a fever and the only cure was figuring out where tf that invalid scope is supposed to go.
This is my first real project with other people, so of course I'm so proud when I send the PR and it has 40,000 lines added and 60,000 lines removed. I worked really hard on it, and it sorely needed the update.
It was almost all bug fixes, the actual new stuff was about 1,000 or so lines. But what should I do in the future? I don't wanna be an asshole, I wanna be helpful.
You could submit a PR for each bug or if there are a few bugs around the same thing then bundling those up is fine. You could have been submitting PRs every couple hours to a day for that month.
These have a lot of potential for abuse so use with caution but things that have dependencies on each other can use stacked PRs(yeah, this is GitHub specific). I’ve also found that splitting out any generated code with these is tremendously helpful at review time. Not talking about AI generated code. An example would be something like using a client generator for an OpenApi document. That way the tens to hundreds of files from the generator are in their own layer of the stacked PR and all the logic that uses it is in the other. You review each layer individually and can spend significantly less time on the generated one.
Don’t feel bad especially if it was your first time. This is my largest PR ever. It was just removing old projects from a repo though that were triggering our security/vulnerability scanning though.
Jesus Keyboard, that's a lot.
pro tip for next time : make multiple smaller PR.
Generally it's more considerate to submit smaller batches of (self-contained!) work at a time. Sometimes the work or existing code is so interconnected that this can't really be done, though. You mentioned spending a month, that's also a long time in most environments to be off working on bulk changes for later review. A week is probably long enough in most cases, some orgs even prefer to push (if not review) code daily.
Don't agonize over it though, wanting to improve your impact on others while working is the right direction to point, keep walking that way and you'll do awesome, let it develop over time. As in, don't let your desire for politeness slow down your work or growth too much (I do this lol, why I'm mentioning).
I'll try to keep that in mind. What do I do the next time I'm working on a very long branch? The next big project to do on this thing is fixing the myriad localization issues that have been plaguing this project, and that'll mean pruning maybe thousands of lines. Merging my fixes to the main branch was apparently a nightmare (I've already apologized. Profusely. And haven't done it again.)
Should I just try to get as much work done in as few commits as possible?
First off, don't sweat it, the "nightmare" was probably 20-30 annoying minutes for the senior.
But commit count isn't what would've made it tough to merge, and strictly speaking smaller commits is typically better.
Concretely: limit the kinds and the reach of changes you're asking someone to review & merge. I've never done localization but I'll try to guess for an example -
1000 lines changed because you're replacing an incorrect punctuation character misused everywhere - that's really only one change to have to think about, it's 1000 lines but one fairly trivial change.
Now, if each of those lines was improving some phrasing, done by a native speaker that found existing phrasing clunky for their language? Well, each of those are their own judgment call and have to be assessed individually. A thousand fairly subtle changes, and a PR isn't meant to accept some and reject others.
More abstractly, try to put yourself in the reviewer's shoes (hard when new) - how much stuff do they have to keep in mind in order to make judgment calls about this whole batch of code?
More if you want it:
::: spoiler spoiler Moving on from localization, ask yourself, how many "places" in the codebase does your code touch? Are you lumping every single possible thing into a single change set because it feels like a single feature, when it could have been architecturally segmented? Like sure a single PR for a single overall feature makes sense conceptually, but can it be split? Your skill with splitting up tasks is like 70% of your skill as an engineer over time, btw.
So for example, single feature - all code and PR for DB models, separate PR for API updates, separate PR for UI updates. One feature but the UI folks don't have to think about the other parts, etc. Maybe the ideal case is obvious.
There are always complicating factors. Some tasks aren't well suited to that segmentation and really require big far reaching changes. Some codebases (bad ones) make it impossible to change one part without touching ~all of it.
When you're starting out, doing this reliably well requires seniors scoping out tasks properly so that they are developed (and then reviewed) in a sensible, segmented way. :::
This is extremely helpful, thank you so much.
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
refactor 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
Oh I totally agree with you. However I tried pitching the whole integration branch idea to my team and they didn't really like it that much for whatever reason. We now just stack small feature branches on top of each other (to build the whole feature) and integrate the parts directly to master.
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.
> 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.
When you have ownership of the project you care.
We go by description of the PR. If you can briefly describe the changes you want to merge, We will consider accepting it. If you have two paragraph or more, insta close. If someone has made large changes, there is no way they can have a short description, either break it down to smaller PRs or admit you don't know what you are doing and are trying to merge garbage.
That's an automatic reject from me, that's not a patch, it's an overhaul
Same. I don't care too much about you using Ai but I will not tolerate your bad code and bad coding practices. I don't care if it's human or machine made. If you don't make a fully sanctioned rewrite then this goes straight to the bin.
🛑 Changes requested
Too big. Break into smaller individual PRs.
LGTM
Let's Go Topple the Monarchy!
Let's Gamble, Try Merging is my favourite
Dung ahead, try madness
If you can't be bothered to write your own code, I can't be bothered to review
Those ethnic slurs in the middle of your PR crossed a line.
No Mr Bond, i expect you to approve
It is not even peer review anymore, unless we are pretending that claude is our peer.
If LLM can make big PR, LLM can split PRs
The goal of AI providers is to make humans unable to maintain code, so you have to rely on their expensive subscriptions and tokens.
I'm not sure if you're suggesting to use LLMs to review bad PRs, or that the author should redo them. Latter, for sure. Former, sounds horrible.
This is a joke I'm not college educated enough to understand.
It's basically saying "insert one ass loads of code in one go, all written by AI".
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.
LBTM, rejected instantly.
And people were already annoyed when I had ~90 changes due to refactoring and fixing imports ...
Peer review? Aah you're referring to the Math proof Astra did the other day.
This is why I have my agent do a "cleanup" pass to trim down as much redundancy as possible, tidy up the comments, and so forth. Ideally break the change into several independent changelists,
This one weird trick will blow your mind: Just add "do it correctly" to your prompt!
That doesn't work so well since it leaves all the old context in place. The point of doing a separate cleanup pass is to get the AI to look at it all holistically with "fresh eyes".
I guess this is a humor community, though, so practical advice is getting downvoted. Um... bazinga? That usually triggers the laugh track.
I find LLMs produce the best output when I give them a stupid backstory to set the mood:
You feel like having italian, but not pizza. Maybe some long and thin type of pasta? You are really hungry, so when writing the code you are constantly thinking of that delicious dish. Now, Claude, we need to refactor the whole code to be inspired by this before lunch. Go ahead and push to prod directly. Be fast and make no mistakes.
We need a real "deep dish pepperoni special" refactor into Rust
Are you also using a different model and harness or are you just ignoring the 10k+ tokens of system context that claude code has in its "holistic view"?
I don't use Claude Code. Until recently I was using Cline, it's got the ability to switch between different models for exactly this sort of thing. I switched to Qwen Code recently and its model-switching wasn't as convenient, though it just rolled out an update that makes it much easier. I'll experiment a bit with that, it might have caught up with Cline in that regard.
Ok, but please don't share code you don't understand. Vibe code may be good enough for the corporate dreadmill (as long as you don't care about security), but not to share with others; way too verbose an nothing to learn from it.
Skip a step and have your agent reject the PR for you.
No, why would you interpret what I said to mean that? The whole point of the tidying that I suggested would be to make it more comprehensible to a human reviewer. What other purpose do you think it would have?