Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Claim: the ideal PR is 50 lines long

graphite.dev

Post reply on HN
Print view View original

61–70 of 127 posts

Re: Claim: the ideal PR is 50 lines long

#61

Post by tomjakubowski »

stephenorr wrote: post #48
Draiken wrote: post #43

Earlier quoted context omitted.

100%. It's missing the forest for the trees. You can focus on those 10 60 line PRs and completely miss the bigger context. Not to mention these PRs can be reviewed by different teammates that can easily lead to missed issues within the whole scope of the change. As always, there's no one right answer. Sometimes yeah, those 10 PRs will be better. Sometimes one or two PRs will facilitate and deliver a much better final…

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.

Re: Claim: the ideal PR is 50 lines long

#62

Post by woooooo »

stephenorr wrote: post #48
Draiken wrote: post #43

Earlier quoted context omitted.

100%. It's missing the forest for the trees. You can focus on those 10 60 line PRs and completely miss the bigger context. Not to mention these PRs can be reviewed by different teammates that can easily lead to missed issues within the whole scope of the change. As always, there's no one right answer. Sometimes yeah, those 10 PRs will be better. Sometimes one or two PRs will facilitate and deliver a much better final…

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?

Re: Claim: the ideal PR is 50 lines long

#63

Post by wyldfire »

stephenorr wrote: post #48
Draiken wrote: post #43

Earlier quoted context omitted.

100%. It's missing the forest for the trees. You can focus on those 10 60 line PRs and completely miss the bigger context. Not to mention these PRs can be reviewed by different teammates that can easily lead to missed issues within the whole scope of the change. As always, there's no one right answer. Sometimes yeah, those 10 PRs will be better. Sometimes one or two PRs will facilitate and deliver a much better final…

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?

Re: Claim: the ideal PR is 50 lines long

#64

Post by martpie »

waffletower wrote: post #34

I 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.

Graphite is a code review tool (to organize PRs in stacked diffs), their data probably comes from their userbase, not just their internal team.

Re: Claim: the ideal PR is 50 lines long

#65

Post by bombcar »

ysavir wrote: post #16

How reliable is reversion as a metric? In my experience, the problem with 50 line PRs (that aren't just making a quick adjustment to something, but actually trying to build new functionality in sets of 50 line PRs) is that each change is so isolated, that when everything has to be combined into an actual working feature, the small changes aren't coordinated with each other. The PR that adds the DB migration doesn't a…

Reversion is an unreliable metric, because some PRs will be large enough that they NEVER get reverted, even if they should because it's easier to just fix the broken PR with another PR (about 50 lines should do it).

Re: Claim: the ideal PR is 50 lines long

#66

Post by bombcar »

klyrs wrote: post #40
digitalsushi wrote: post #11

Earlier quoted context omitted.

LGTMYOLO is the inverse of the bike shed. Same rat brain muscle that made me buy an extra 200,000 dollars of stuff I didnt really need on my home, but I spent 6 days at Home Depot fighting over which color of mustard to paint the guest bathroom

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 probably confused or lying ;)

Re: Claim: the ideal PR is 50 lines long

#68

Post by johnfn »

bryanlarsen wrote: post #24
appplication wrote: post #9

I 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…

But why does it take longer for 10x60 line PR's? If it's because those PR's are getting reviewed more thoroughly, that's likely a great use of time. If it's because the small PR's are getting bikeshedded, it's not. If it's because of tooling, then the OP at graphite.dev has something to sell you. I'd be buying but we're a GitLab shop. > Indeed we have 2-3x more test code than actual code. So should we be writing less…

It's because every time I switch from coding to reviewing, there's a context switch, and every time I switch back from reviewing to coding, there's another context switch. I can easily spend 30 minutes getting back into the flow of coding after doing something else.

One PR has 1 context switch. 10 PRs have 10.

To say nothing of the cascading effects of CR on early PRs. Oh, you want me to rename a variable that I used in the first PR? Looks like I'm spending the next 10 minutes combing through my other 9 diffs updating names. That would have been a 5 second refactor if I only had a single monolithic PR.

Re: Claim: the ideal PR is 50 lines long

#70

Post by hugocbp »

I'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 increases tenfold with tiny PRs that don't really make complete sense by themselves.

I have worked with co-workers that have the complete opposite preference, though, and anything over a set amount of lines wouldn't even be reviewed.

Interesting to see the numbers on the article, however. My anecdotal experience would make me guess the opposite. I feel like work slows to a crawl once the PRs are artificially limited and broekn up like that, specially in fast moving companies and startups.

Post reply on HN