Claim: the ideal PR is exactly as long as it needs to be to achieve its goal.
The same goes for functions
Please stop listening to this garbage advice.
Claim: the ideal PR is 50 lines long
81–90 of 127 posts
Re: Claim: the ideal PR is 50 lines long
#82Earlier quoted context omitted.
An ex-manager (ex-Google, too) insisted we move from GitLab to Gerrit, and mandated that all commits be a maximum of 5 lines, ideally fewer. This led to a complete loss of all context; without that overview, you might very well be releasing code faster, but it could be the wrong solution to the original problem - and crucially, you won't know until it's out in the wild. A preference for smaller, more focused PRs is f…
This must be a typo - 50 or 500 lines maybe? ...or satire?
I wish it had been satire. This was fundamentally one of the worst parts of my coding career, and it was disappointing because the company had been amazing to work for prior to this individual's arrival.
Post-arrival, I was ordered to rewrite a working, stable PHP web app into Go ("Google doesn't use PHP, because it doesn't scale"). We weren't allowed to write RESTful APIs any more, everything had to use Protobuf.
And there was a physical fight in the office one day between two of the developers, that culminated in one of them storming out and never coming back.
Re: Claim: the ideal PR is 50 lines long
#83Earlier quoted context omitted.
> mandated that all commits be a maximum of 5 lines, ideally fewer Yeesh. How would you ... rename anything? Imagine a linter rule which prevents referencing a given name on more than 5 different lines of code.
Many language allow an alias name. Which means you change the name and then alias the old name to the new line - 2 line PR. Then a bunch of one line PRs to change easy use of the old name. Finally a PR to remove the alias. Which is why I don't have such a rule and would oppose it.
The attitude might have been something to do with the individual's SRE background. In that context, I understand the desire to ship small changesets that have a limited blast radius.
It's just not really practical for actually building software.
Re: Claim: the ideal PR is 50 lines long
#84I'm probably in the minority here, but personally I'd much rather review a 300 line PR instead of 6 50-line ones if the change is a single context. I briefly worked with a hard line-count-limit for PRs and I thought it made everything much worse. It is fine for changes that are actually small, but when you need to go back and re-open 4, 5 merged PRs in different tabs to get the full context again, the time to review…
Line count (or count of any units in the PR) is completely meaningless.
I feel that in the last decade somehow there has been a loss of context of what is the purpose of version control.
Why do we bother with version control? Sure, there are many reasons.
But a primary reason is that if we discover behavior X turns out to cause regressions (or is otherwise a bad idea), we can easily revert it by reverting its commit.
That's a why a single commit should contain a single behavior change, and contain the entirety of that single behavior change.
If the change is split among multiple commits, it'll be a pain to revert it. If the change is contained within a commit that changes other things, it'll be an ever larger pain to revert it.
So that's my rule. A commit is single behavior change, all of it, and nothing else.
You can't express that in lines. Might be 1 line might be lots.
Re: Claim: the ideal PR is 50 lines long
#85This is one of those cases of where we're focusing on the wrong metric for a very generic problem.
There's too many facets to what makes a PR good that boiling it down to line count is silly.
Ex. Small code changes are required if the solution requires it - forcing 5 lines change to 50 lines would be considered wildly bad idea. On the other hand a 500 line PR to solve one problem suddenly broken down to 10 PRs makes the review far harder depending on the context.
Re: Claim: the ideal PR is 50 lines long
#86Earlier quoted context omitted.
You missed your parent's point: LGTMYOLO is the response to large PRs; where small PRs get bikeshedding. And this is the classic theme behind bikeshedding. People can understand that small PR at a glance, and there's always multiple ways of doing everything. There's a perverse desire to demonstrate productivity, and nagging at small PRs is a way to do that without a lot of effort; ironically reducing actual productiv…
That's exactly what happens. At some point a PR gets too big and the only thing you can do is run the tests, try the functionality, and LGTMYOLO approve it. You leave it open longer in hopes someone else will look at it, and maybe one or two people look at places they've had issues in the past, but it gets through and blows up spectacularly and those get fixed. Anyone who says their development is not the above is pr…
I've participated in month-long reviews where nobody's time was wasted. That's extremely rare, mind you, but if you're accustomed to low-quality code and a "break shit fast" attitude, you might be inclined to mistake that for a universal rule of software development.
In fact, if you put quality first, the "only thing" you can do with a long PR is actually do the work. And the more accustomed you and your team become with actually doing work, the more productive you'll be.
Re: Claim: the ideal PR is 50 lines long
#87I'm probably in the minority here, but personally I'd much rather review a 300 line PR instead of 6 50-line ones if the change is a single context. I briefly worked with a hard line-count-limit for PRs and I thought it made everything much worse. It is fine for changes that are actually small, but when you need to go back and re-open 4, 5 merged PRs in different tabs to get the full context again, the time to review…
> but when you need to go back and re-open 4, 5 merged PRs in different tabs to get the full context again, the time to review increases tenfold with tiny PRs
You can't really say much about the first 4 PRs because if you don't know how that code is going to be used, it's kind of hard to giving meaningful feedback. By PR 5, it's too late to give feedback on PRs 1-4.
I also have worked with people that prefer that but I never understood it. Code without context isn't reviewable imo
Re: Claim: the ideal PR is 50 lines long
#88Just use your brain to PR a feature or some sub-piece of a feature instead of this arbitrary shit.
Who wants to spend hours bothering with hardline process - you’re the programmer, you’re going to be the guy answering any questions on your pull; therefore you’re going to have to look at it yourself.
Re: Claim: the ideal PR is 50 lines long
#89I file this under the category of "needless rules created without sufficient context". The author's experience is not sufficient to generalize to the diversity of development environments where pull requests are utilized. Hopefully, OP is a good listener and can compromise when their strong opinions meet resistance from other developers at their organization.
Also file under “product advertisement”.
Re: Claim: the ideal PR is 50 lines long
#90I'm seeing some pretty surprising claims in this thread about, "I prefer reviewing larger PRs," and, "Teams that make larger PRs go more quickly." That all seems like rubbish to me. I've seen data indicating smaller PRs having lower error rates and such and then we have this article with data recommending a 50 line PR size. I've *never* seen data show large PRs are better except for some developers saying, "I prefer it that way."
Anyways, I don't so much care about the size of a PR as much as I care about how understandable it is. That generally ends up being a few rules of thumb like:
1. If you change a common function name, change it everywhere but please don't do anything else in the same commit.
2. If you need to make a change in code you are depending on to accomplish a goal in your own code, try to break the change to the dependency into another commit. There is a little bit of an art here to both understand how to break things up and to not break them up too small.
3. Try to make your history meaningful rather than just having a bunch of random commits like "fix", "forgotten file", etc.