ITT: company that sells product for breaking PR into smaller PRs claims smaller PRs are better.
Not saying the claim is necessarily wrong, but it's also exactly what I'd expect given the source.
Claim: the ideal PR is 50 lines long
71–80 of 127 posts
Re: Claim: the ideal PR is 50 lines long
#72Earlier quoted context omitted.
There's no context in this to say what kind of change that is referring to. I can make hundreds of 2 line PRs fixing typos and reformatting text or whatever else low stake change that are as small as this. So, yeah, obviously those low-stake changes that don't really do much will be quick to review and merge. Does that make them any better? Why are we optimizing for review speed alone in the first place? It's just si…
Okay, let's try for a more concrete example. Suppose you're writing a new feature, and the bulk of your PR is a one-off 1,000-line function that does ten different things in sequence. If you can factor out that big long function into ten smaller functions that are each useful in isolation, then your code is probably going to be a lot easier to understand, regardless of how your structure your commit history. Future c…
Much more common in my experience is a 10 line function that relies on 30 lines of new SQL schema, both of which are used in an 80 line report.
Context matters. Reviewing any one of the 3 without simultaneously looking at the other 2 is folly.
Re: Claim: the ideal PR is 50 lines long
#73Earlier 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…
> 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.
Re: Claim: the ideal PR is 50 lines long
#74I have found that teams who allow larger PR sizes without complaining about it can get a lot done a lot faster. Which is to say having 50 line PRs are not ubiquitously ideal, but are somewhere on the spectrum of acceptable but come with definitive tradeoffs. Reviewing and merging 10x60 line PRs is, in my experience, more time intensive than reviewing one 600 line PR. Most of the tests for and new functionality alone…
I couldn't disagree more. My current team has a history of large PRs and everything was a dumpster fire until we started really hammering down on PR size. Prod broke all the time, reverting code was a nightmare because of how big the "units" were, code review was not thorough, problems problems problems. > large PRs are harder to understand but that’s why you have tests I'd rather have comprehensible, well-reviewed c…
My team has a history of large (and small) PRs. If it's a huge feature / vertical, it's often done in a few large PRs. Very few reverts in our 5 year history.
I guess, what I'm saying is, it depends on the team, on their seniority and capabilities, on the product, the culture, etc.
Re: Claim: the ideal PR is 50 lines long
#75I have found that teams who allow larger PR sizes without complaining about it can get a lot done a lot faster. Which is to say having 50 line PRs are not ubiquitously ideal, but are somewhere on the spectrum of acceptable but come with definitive tradeoffs. Reviewing and merging 10x60 line PRs is, in my experience, more time intensive than reviewing one 600 line PR. Most of the tests for and new functionality alone…
Measuring the total size of the PR seems to me to be missing a dimension, namely the commit size. What if a 600-line PR consist of 10 60-line reviewable commits? Which, if not for GitHub’s broken review UI, is what I would hope is the goal for such a thing. Then you have a reasonable amount of code to look at per commit, and all in context of the larger overall change.
Re: Claim: the ideal PR is 50 lines long
#76I'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…
Lines with actual code changes aren't a problem for me, it's the automated IDE indent/spacing/bracketing that really drives me up a wall and fatigues the hell out of me.
But this might only be a problem with those of us working on legacy codebases. The kind of PRs I see in OSS projects I could review 1000 lines at a time - it's so clean!
Re: Claim: the ideal PR is 50 lines long
#77For me personally, it shouldn't be about line count. A PR should have scope of a single feature/piece of functionality. Or if it is a PR for a bug fix, 1 bug fix per PR. If its 1 line, 2 lines, 50 lines, 500 lines, I don't care. Each PR should address one single problem.
Re: Claim: the ideal PR is 50 lines long
#78I'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…
I tend to agree.
For my part, ever since I was a wee lad learning to program for the first time, I've been of the opinion that line counts themselves are a largely worthless metric for anything.
How many lines code takes up is too variable between languages, too variable within the same language, too dependent on irrelevancies like formatting, etc.
What actually matters is logical and conceptual complexity, not line counts.
Re: Claim: the ideal PR is 50 lines long
#79I'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…
My bar is if its more than 20 files, or breaks the version control UI, which I've seen in bitbucket some years ago, then your PR is a little too much, and if its a major automated refactor of some sort, it should not contain any other code changes outside of what's necessary to make that specific change work as intended. Other changes should be their own discrete PRs as follow ups.
I just try to be reasonable about PRs. Everyone will have their preference, I just think as long as you can go back and figure out what happened, why the change was made, and do it quickly enough, then you're golden.
Re: Claim: the ideal PR is 50 lines long
#80Earlier 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…
5???? Please tell us more. Did 6 line commits get rejected? How did anything get done?
Well, things did get done, but not the right things.
The rule wasn't hard and fast, but you had to have a damn good reason why you wanted to add bigger commits. Protobuf definitions didn't count, so you could get away with doing longer commits for those.
Tests were discouraged anyway ("Google just tests in production", we were told), so there wasn't a lot of that sort of thing.