top | item 47023530

(no title)

rokkamokka | 15 days ago

I'm on the fence about this. Sometimes a new feature needs maybe 2k lines of code split over 10-20 or so files. Sure, you could split it up, but you can't necessarily run the parts in isolation and if they get split over multiple reviewers it might even be that no one reviewer gets the whole context.

So, I'm kind of okay with large PRs as long as they're one logical unit. Or, maybe rather, if it would be less painful to review as one PR rather than several.

discuss

order

captainbland|13 days ago

I think the particular problem is if AI is just producing large volumes of code which are unnecessary, because the LLM is simply not able to create a more concise solution. If this is the case it suggests these LLM generated solutions are likely bringing about a lot of tech debt faster than anyone is ever likely to be able to resolve it. Although maybe people are banking on LLMs themselves one day being sophisticated enough to do it, although that would also be the perfect time to price gouge them.

alan-stark|13 days ago

Agree. We've seen cowboy developers who move fast by producing unreadable code and cutting every corner. And sometimes that's ok. Say you want a proof of concept to validate demand and iterate on feedback. But we want maintainable and reliable production code we can reason about and grasp quickly. Tech debt has a price to pay and looks like LLM abusers are on a path to waking up with a heavy hangover :)

strken|13 days ago

I'm okay with long PRs, but please split them up into shorter commits. One big logical unit can nearly always be split into a few smaller units - tests, helpers, preliminary refactoring, handling the happy vs error path, etc. can be separated out from the rest of the code.

nh2|13 days ago

There really is no benefit of splitting a functionality from it's test. Then you just have a commit in the history which is not covered by tests.

Splitting "handling the happy vs error path" sounds even worse. Now I first have to review something that's obviously wrong (lack of error handling). That would commit code that is just wrong.

What is next, separating the idea from making it typecheck?

One should split commits into the minimum size that makes sense, not smaller.

"Makes sense" should be "passes tests, is useful for git bisect" etc, not "has less lines than arbitrary number I personally like to review" - use a proper review tool to help with long reviews.

sublinear|13 days ago

> Sometimes a new feature needs maybe 2k lines of code split over 10-20 or so files

I still disagree. Why was the feature not split up into more low-level details? I don't trust that kind of project management to really know what it's doing either.

I am not promoting micromanagement, but any large code review means the dev is having to make a lot of independent decisions. These may be the right decisions, but there's still a lack of communication happening.

Hands off management can be good for creativity and team trust, but ultimately still bad for the outcome. I'm speaking from my own experience here. I would never go back to working somewhere not very collaborative.

cik|13 days ago

This is very much my take. As long as the general rule is a lack of long PRs, I think we get into a good place. Blueskying, scaffolding, all sorts of things reasonably end up in long PRs.

But, it becomes incumbent on the author to write a guide for reviewing the request, to call the reviewer's attention to areas of interest, perhaps even to outline decisions made.

sublinear|13 days ago

> on the author to write a guide for reviewing the request

I'm not saying that doesn't work, but writing a guide means the author is now also doing all the planning too.

A successful "guide" then becomes more about convincing the reviewer. The outcome is either a lot of friction, or the reviewer is just going through the motions and trust is eroding.

dboreham|13 days ago

That's fine but such a PR doesn't need to be (and actually can't) be reviewed. Or at least it can only be reviewed broadly: does it change files that shouldn't change, does it have appropriate tests, etc.

fastasucan|13 days ago

Maybe AI should not author big and complex features that cant be split up into parts and thus easily reviewed.