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

31–40 of 127 posts

Re: Claim: the ideal PR is 50 lines long

#31

Post by Draiken »

What a weird claim.

PRs that are Most real value add PRs end up bigger due to tests, documentation changes, and the actual change.

This for me is the typical claim that'll become an example of Goodhart's law when implemented. Random people that are not engineers will use this to claim "your PR should be 50 lines long" as some sort of absolute truth, when in reality the number of lines of code is absolutely irrelevant most of the time.

Context and details matter. Sometimes multiple small PRs are better. Sometimes a larger PR is better. It always depends. These absolutist claims that "this is better" is what leads to atrocious management practices that always end up getting summarized to a number, losing all of its meaning in the process.

Had we not we moved on from the lines of code metrics already decades ago? :facepalm:

Re: Claim: the ideal PR is 50 lines long

#32

Post by 3cats-in-a-coat »

okwhateverdude wrote: post #29
3cats-in-a-coat wrote: post #18

Just my comments are 50 lines, but thanks I'll never stop finding these "functions must be this long and classes must have that many properties, and PR should be so many lines" etc. rules hilarious. It cheers me up that people so naive exist. Size is contextual to the thing the PR regards. Maybe it's one char. Maybe it's 500 lines. Maybe it's 10k lines. Screw this.

Seems like a skill issue, honestly. I feel like many of these arbitrary rules end up being made to account for low skill developers to curb their behaviors which seems like the wrong way to tackle that problem.

Precisely

Re: Claim: the ideal PR is 50 lines long

#33

Post by ysavir »

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…

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 comparing notes between them to make sure you really understand any of them.

I still think 600 is a lot, but there's a happy medium between "minimal change that excluded broader context" and "monster PR that has all context, all changes, and eats left-pad packages for breakfast". Line count shouldn't be the goal. The goal should be making as small a change possible that is self-contained (ie all context is either within the PR or already in the codebase), and (IMO) that can be delployed to production as-is.

Re: Claim: the ideal PR is 50 lines long

#34

Post by waffletower »

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.

Re: Claim: the ideal PR is 50 lines long

#36

Post by HPsquared »

TomSwirly wrote: post #6

> 50-line code changes are reviewed and merged ~40% faster than 250-line changes. Reviewing a 50-line code change in 60% of the time it takes to review a 250-line code change means the shorter code review takes _four times_ as long to review per line.

Also splitting the 250-line change into 50-line parts will probably involve more total lines - to ensure it can be done piece-by-piece then fit together later.

So not only is it more time per line, it's also more lines.

Basically the only benefit is PRs per day, which is... not useful in itself.

Re: Claim: the ideal PR is 50 lines long

#37

Post by NickM »

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, yes, it can be a bit more work to split up big PRs into smaller standalone changes that can be merged in isolation, but it seems plausible that the benefits might outweigh the extra work in many cases. The whole idea of a version control system is to create a useful history of changes over time; making each individual changeset simpler and easier to understand seems like an admirable goal.

Re: Claim: the ideal PR is 50 lines long

#39

Post by Draiken »

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 silly statistics being thrown around. There's no correlation in lines of code or review speeds for value added.

A larger PR that actually does something and requires more thorough review will inevitably take longer. Does that mean it would be better to have 10 separate PRs with no connection between them, different reviewers and 10x the feedback loops? This for me is missing the forest for the trees.

This honestly reminds me of way back when Agile/Scrum was the new hot thing, and my company implemented it where we ended up having 10x the stories taking twice the time.

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

Re: Claim: the ideal PR is 50 lines long

#40

Post by klyrs »

digitalsushi wrote: post #11
lakpan wrote: post #5

I wish. My small PRs die by a thousand bikesheds. My large refactors are followed by LGTMYOLO

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 productivity, see: Cobra Effect.

That big PR though? Saying something intelligible about it requires serious effort. In a low-skill environment, nobody will touch the PR for fear of demonstrating that they don't know what they're talking about.

I disagree with the premise of the article. The ideal PR is a well-encapsulated unit of work. Sometimes you need to fix a typo or handle an edge case, and you make a 1-line PR. Sometimes you need to add a new module and you add 3 thousand-line files and put little hooks in 30 more files.

Post reply on HN