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

101–110 of 127 posts

Re: Claim: the ideal PR is 50 lines long

#101

Post by johannes1234321 »

klodolph wrote: post #98
jvans wrote: post #87

Earlier quoted context omitted.

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

A skill that you see in some of the more experienced devs is the ability to front-load the most important stuff in a series of changes, or make changes that can at least be understood in isolation. It’s not always reasonable to do that, but you can often deliver incremental changes where each incremental change either brings a whole feature visibly closer to completion, or provides some self-contained utility, or mak…

Right, in my experience large changesets often don't add a single thing, but do some refactoring, which enables the change, and then add some new infrastructure and the individual feature.

If you split it, you can verify the refactoring is a pure refactoring, not changing functionality (much), can understnad the infrastructure added and then understand the feature built on top.

Of course often things aren't developed in isolation, thus preparing the review for isolating those aspects is work again, but usually that in a side effect leads to better architecture, as you look at the parts in isolation, leading to half a refactoring, half an infrastructure and a feature hacked in.

Re: Claim: the ideal PR is 50 lines long

#102

Post by ffgjgf1 »

wk_end wrote: post #41
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…

In re: to > Reviewing and merging 10x60 line PRs is, in my experience, more time intensive than reviewing one 600 line PR. The article disagrees, and the article has data: > Some folks might wonder if writing small PRs leads to less code output in total. We all want to be high velocity, but there are times when you need to be high volume. Consistently writing sub-20 line PRs will have a significant impact on your net…

> and the article has data

How does that data prove anything? The ideal PR size is 50 lines because that’s the median size on Github. Seem like a pretty worthless claim.

Re: Claim: the ideal PR is 50 lines long

#104

Post by starttoaster »

klodolph wrote: post #98
jvans wrote: post #87

Earlier quoted context omitted.

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

A skill that you see in some of the more experienced devs is the ability to front-load the most important stuff in a series of changes, or make changes that can at least be understood in isolation. It’s not always reasonable to do that, but you can often deliver incremental changes where each incremental change either brings a whole feature visibly closer to completion, or provides some self-contained utility, or mak…

That's a talented approach, yes. But something I've observed in a couple of former coworkers is they will put together 6 PRs to the main branch where the code won't even (purposefully) build yet. By PR 6, the code functions, but is in a terrible state because PRs 1-5 were essentially pointless to review, and many of the issues you have with the code base are now firmly outside of the 6th PR's diff. I've likened this phenomena to the H.H. Holmes murder hotel, where he hired contractors to do extremely small jobs individually, and none of them realized (or at least had some plausible deniability) that they were creating a hotel made perfectly for the owner to murder residents. Except in this situation, I perceive the author was hiding the fact that they have no clue how to write code, which I don't know who they thought they fooled.

Re: Claim: the ideal PR is 50 lines long

#105

Post by wk_end »

ffgjgf1 wrote: post #102
wk_end wrote: post #41

Earlier quoted context omitted.

In re: to > Reviewing and merging 10x60 line PRs is, in my experience, more time intensive than reviewing one 600 line PR. The article disagrees, and the article has data: > Some folks might wonder if writing small PRs leads to less code output in total. We all want to be high velocity, but there are times when you need to be high volume. Consistently writing sub-20 line PRs will have a significant impact on your net…

> and the article has data How does that data prove anything? The ideal PR size is 50 lines because that’s the median size on Github. Seem like a pretty worthless claim.

> The ideal PR size is 50 lines because that’s the median size on Github.

???

No, that's not why it's arguing that it's the ideal size. Where did you get that idea? (TBH I doubt it's the case - apparently the average PR size on GH is nearly 1000 lines [0])

It's arguing that it's the ideal size because (1) PRs of that size end up getting reviewed and merged the fastest; (2) PRs of that size end up getting reverted the least, implying that they're less likely to cause breakage; (3) PRs of that size end up getting more review comments, implying higher engagement and less rubber-stamping (there's a concern that this could imply unnecessary bikeshedding that slows things down, but see 1); and (4) PRs of that size end up creating the highest total throughput.

I suppose that doesn't "prove" anything, because correlation doesn't imply causation. But it gestures towards it.

[0] https://www.keypup.io/product/average-pull-request-size-metr...

Re: Claim: the ideal PR is 50 lines long

#106

Post by wk_end »

martpie wrote: post #64
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.

Yes, it says as much.

> Our sample set

> All of the data-based statements in this piece are made using private and public PRs and repos that have been synced with Graphite.

Re: Claim: the ideal PR is 50 lines long

#107

Post by bvrmn »

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…

Refactorings and redesings could easily span 1k lines even for small projects. Yes, almost always you could make it incremental, but total lines counts would increase and it requires hard planing beforehand. Too much fuzz.

Re: Claim: the ideal PR is 50 lines long

#108

Post by bvrmn »

herpdyderp wrote: post #57
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…

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…

> everyone is scared to change because of how much of a mess it is

As a rule messy code a way more easier to cleanup in one big chunk. Teams with CR limit prefer not to touch such nests.

Re: Claim: the ideal PR is 50 lines long

#109

Post by bvrmn »

PR line count is such a silly metric. For example my recent PR +600/-900 lines. What you can deduct from that? Is it complex?

Changes were quite simple. 600 common lines are about rearranging big module into multiple smaller modules. 300 removed lines are technically could be in a separate PR, but it's some cleanup aftermath related to rearrangement.

Re: Claim: the ideal PR is 50 lines long

#110

Post by joshka »

degenerate wrote: post #76
hugocbp wrote: post #70

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…

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!

Split the reformatting and real change. Make the reformatting change first and have a policy of merging those change fast.
Post reply on HN