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

51–60 of 127 posts

Re: Claim: the ideal PR is 50 lines long

#51

Post by wk_end »

Draiken wrote: post #45
danielovichdk wrote: post #27

Earlier quoted context omitted.

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

The article also claims it's the ideal size because it minimizes reverts and ends up with PRs getting more thoroughly reviewed (based on # of comments). I think that makes for a more compelling case.

Re: Claim: the ideal PR is 50 lines long

#52

Post by karmakaze »

Large and small PRs aren't the only options.

For a large PR, I usually regroup my changes into smaller unit commits which can be reviewed commit by commit.

For an even larger (or more complex) PR, I'll make a set of stacked PRs which can be reviewed in pieces, each with a good description of what/why/how stuff is happening in that piece. Once all PRs have been approved, the lot of them can be deployed together.

I've at times had 3 PRs for a total of 3 line changes, because they were for a single table's schema migration picking new indexes and each had many factors to consider. The PR descriptions were of course much longer.

Re: Claim: the ideal PR is 50 lines long

#53

Post by Draiken »

wk_end wrote: post #51
Draiken wrote: post #45

Earlier quoted context omitted.

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.

The article also claims it's the ideal size because it minimizes reverts and ends up with PRs getting more thoroughly reviewed (based on # of comments). I think that makes for a more compelling case.

Not unless we know what those changes are.

Guess what, all typo commit fixes are <50 lines and never get reverted.

Re: Claim: the ideal PR is 50 lines long

#54

Post by bryanlarsen »

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…

That each MR be deployable is a great guideline and should trump any sort of line-number guideline. It's possible that an MR could be deployable but lack context for the bigger change, but it's less likely.

Re: Claim: the ideal PR is 50 lines long

#55

Post by bryanlarsen »

Draiken wrote: post #53
wk_end wrote: post #51

Earlier quoted context omitted.

The article also claims it's the ideal size because it minimizes reverts and ends up with PRs getting more thoroughly reviewed (based on # of comments). I think that makes for a more compelling case.

Not unless we know what those changes are. Guess what, all typo commit fixes are <50 lines and never get reverted.

I wish never, definitely have been burned by a thinko in a supposed "typo fix". But I'll give you "rarely".

Re: Claim: the ideal PR is 50 lines long

#56

Post by Draiken »

bryanlarsen wrote: post #55
Draiken wrote: post #53

Earlier quoted context omitted.

Not unless we know what those changes are. Guess what, all typo commit fixes are <50 lines and never get reverted.

I wish never, definitely have been burned by a thinko in a supposed "typo fix". But I'll give you "rarely".

Haha, that's a fair point. Cries in dynamic languages

I've definitely had code with typos consistently replicated due to auto completion, then had to fix it all later on, haha.

Re: Claim: the ideal PR is 50 lines long

#57

Post by herpdyderp »

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 code than test coverage (I'd rather have both, of course). We've got a mission critical module that is a crusty piece of junk that everyone is scared to change because of how much of a mess it is, even with its amazing test coverage (frankly the code is so bad that I assume the tests are just as bad anyway).

> XXL PRs aren’t usually happening every day. If they are, you have a very productive team and maybe you should count yourself lucky

I've never seen an XXL PR that improved productivity. They pile up tech debt, bugs, and down time. I don't care how fast the devs are moving if they bring prod down every week. That is absolutely unacceptable.

On a team of superstar, perfect developers that all understand the code base perfectly, I'd probably agree with you. If that's your situation then I envy you :) But I've never been on such a team.

Re: Claim: the ideal PR is 50 lines long

#58

Post by xkcd-sucks »

> If what you care about is maximizing the amount of engagement and feedback on your code over the long run, you’re best off writing as small of PRs as possible.

> The highest volume coders and repositories have a median change size of only 40-80 lines.

This claim is rational under the Facebook axioms, so to speak.

Post reply on HN