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

41–50 of 127 posts

Re: Claim: the ideal PR is 50 lines long

#41

Post by wk_end »

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 coding ability - but interestingly, so will writing PRs greater than 100+ lines. The highest volume coders and repositories have a median change size of only 40-80 lines. I suspect this is because the volume of code is change-size * speed of changes. Too small of changes, and the faster merge time doesn't make up for it. Too large, and you start getting weighted down by slower review and merge cycles.

I agree that you shouldn't be dogmatic about PR size - there's an art to engineering as much as it's a science. But part of that art might also be recognizing that "unit change, whatever that means to you" is - as you suggest - a flexible concept. The takeaway for me here is, given what the data shows, when you end up with a 600-line unit it might be worthwhile to try to, if possible, find a way to break that unit down.

Re: Claim: the ideal PR is 50 lines long

#43

Post by Draiken »

ysavir wrote: post #33
bryanlarsen wrote: post #24

Earlier quoted context omitted.

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…

Not the OP, but in my experience, it's because those 60 line PRs lack a lot of context, and it's not until you see the cumulative changes across those 10 60-line PRs that you really understand how they come together to form a feature--and are able to review them within that context. So instead of having one 600 line PR in which the whole picture is clear, you have to go back and forth between 10 different PRs and com…

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 solution with less bugs.

It really bothers me how these articles/companies try to claim "ideal" scenarios completely devoid of context. That's how bad managers will use your PR sizes to say you're doing a "bad job" despite not knowing a single thing about coding.

Re: Claim: the ideal PR is 50 lines long

#45

Post by Draiken »

danielovichdk wrote: post #27
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 have found that teams who allow larger PR sizes without complaining about it can get a lot done a lot faster". Sure. How many times is the code touched after the PR has been merged is a better question. Faster is not better. Better is not faster.

Weirdly, what the article claims is that it's the ideal size because of speed. So they are trying to say it's faster, therefore it's better.

Which is in my view a very silly claim devoid of any context.

Re: Claim: the ideal PR is 50 lines long

#46

Post by detinho »

I always try to care about the reviewers. Not too large PRs so they don't LGTM it instantly. Not too small PRs so they don't have to switch contexts multiple times a day or search through many smaller PRs to get context why new PR changes specific lines.

Re: Claim: the ideal PR is 50 lines long

#47

Post by NickM »

Draiken wrote: post #39
NickM wrote: post #37

Lots of commenters seem to be arguing against straw men. The article doesn't say "you need to follow this exact rule on all your PRs"; it doesn't frame any of this as a hard limit or a "best practice", it's just saying that small PRs are easier to understand and review. This doesn't seem terribly controversial to me. The fact that the article actually includes data in support of this thesis is icing on the cake. Now,…

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 code reuse is also easier. Testing each function in isolation may be simpler as well.

Once you do all of that, putting each of the standalone functions in its own commit is trivial, and each commit can have a more focused commit message. Reviews are easier too, since you can easily view each function and its associated docs/tests/etc separately.

Local optimization on these sorts of metrics are pretty much always going to lead to perverse outcomes.

Again, this is a straw man. Nobody is saying to blindly optimize to the metrics, or that this is a black-and-white thing where every commit needs to fit within a hard line limit. But, all else being equal, I've personally found that my code quality improves when I try to break things into smaller independent changes. I also find it much easier to review PRs where other people do that.

Re: Claim: the ideal PR is 50 lines long

#48

Post by stephenorr »

Draiken wrote: post #43
ysavir wrote: post #33

Earlier quoted context omitted.

Not the OP, but in my experience, it's because those 60 line PRs lack a lot of context, and it's not until you see the cumulative changes across those 10 60-line PRs that you really understand how they come together to form a feature--and are able to review them within that context. So instead of having one 600 line PR in which the whole picture is clear, you have to go back and forth between 10 different PRs and com…

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 fine - it certainly makes them easier to review independently - but I think putting any sort of limit on the size makes your team more likely to omit things like tests and also to not have sufficient context to understand the overall problem space.

Re: Claim: the ideal PR is 50 lines long

#50

Post by Draiken »

NickM wrote: post #47
Draiken wrote: post #39

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

I'm not arguing that big PRs are better. I'm arguing that being smaller also doesn't make it better. Neither has any real meaning without context.

Take your same 1000 line change example, but make that change span multiple areas of the code that are related, but not immediately apparent. If you blindly split that up, you can very easily let bugs slip in even if in isolation each change looked fine.

My whole point is: there is no "ideal" size. It depends. It always depends. We should never substitute thinking with mere rules of thumb and reductionist claims.

On another point, notice you said: commits, not pull requests.

That's an important distinction. Pull requests have a whole feedback loop on top of them, and serve exactly as a way to tie multiple smaller changes required to achieve one meaningful change. That means most of the time we can get all the benefits of small code changes and the benefits of the full context by having a slightly larger PR and separating changes by commits.

> Again, this is a straw man. Nobody is saying to blindly optimize to the metrics

Maybe. But this is an advertisement blog post, made for a company that targets managers that will say "let's buy this and it'll make our developers faster. It gives me a pretty graph to say who's good".

That's why some of it might look like a straw man at first sight, but if you take in the context in which this blog post was written, it's a valid argument in my view.

It's way too easy to blur real code concerns (splitting functions, naming, separation of concerns, etc) with silly meaningless numbers (PR size, line counts, PR numbers) that managers use as a proxy to actually knowing the subject matter.

Post reply on HN