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

111–120 of 127 posts

Re: Claim: the ideal PR is 50 lines long

#111

Post by the1024 »

We went through a relatively controversial process w.r.t. enforcing this at my last co. What it really came down to was intent vs. impact - the intent of the change was to enforce better development practices, especially across junior engineers that had shipped monster prs containing multiple bugs that were extremely hard to catch ("the easiest way to ship a one line change is in a thousand line pr").

This worked reasonably well in terms of forcing junior engineers to reason harder about what an atomic change should look like, but also impacted senior folk that were making atomic changes that encompassed broad areas of the codebase. We eventually reverted the change and relied on implementing CODEOWNERs to enforce senior devs to request changes when PRs got out of hand.

There's definitely nuance to enforcement of best practices across an entire organization; haven't really seen it done well although I've primarily worked at startups.

Re: Claim: the ideal PR is 50 lines long

#113

Post by da39a3ee »

This all depends on whether you believe in merging unfinished work to master, or whether you believe features should be completed and tested before merging. Personally I'm in the latter camp, but some of the big tech companies seem to have a style of merging commits representing partial progress toward a feature.

Re: Claim: the ideal PR is 50 lines long

#114

Post by mewpmewp2 »

danpalmer wrote: post #15
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.

There are two ways of looking at this, I think the author took the positive way, and you've taken the negative. You could argue that shorter PRs are easier to review and therefore people are doing more review, and that should result in better changes (indeed the 15% fewer reverts supports this). Or you could argue that shorter PRs are harder to review as they have less context, and therefore more time is taken but th…

It is also going to be more reverts.

If you split 200 lines of code into 4 x 50 you are actually going to have 0.85 x 4 reverts or 3.4x reverts with this strategy.

But overall the whole data is terrible and infuriating because likely the changes are very different in the dataset depending on the loc.

Re: Claim: the ideal PR is 50 lines long

#115

Post by jvans »

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…

Definitely. The good version of small PRs are small but useful changes that incrementally produce value on their own

The bad version is people adding random unused classes and methods that don't make sense until the final PR comes in and uses them

Re: Claim: the ideal PR is 50 lines long

#116

Post by brailsafe »

klodolph wrote: post #97
johnfn wrote: post #68

Earlier quoted context omitted.

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…

Yeah—on the other hand, I’ve gotten PRs that are just too damn big and that means that I can’t review them without sitting down and getting into the flow just for that review. What I end up doing is, after some false starts, getting myself a block of time to review the massive PR. Let’s say I just finished up some change I’m working on, and I take a look at my coworker’s PR. It’s 11:45, and I’m heading to lunch in 15…

I totally agree, and this was a point of conflict between my last manager and I. On one hand, he'd be on my case for not getting some coding done, and on the other he'd be on my case for not jumping on a massive PR right away. Part of the problem I think was that we had a very bottlenecked QA process and decoupled feedback systems for reviewing and discussing.

I'd get a large PR and some smaller ones on GitHub that would likely have tests included, so I'd have to check those too, while also maintaining conversation over on Jira about some other ticket and trying to actually get code written. There was no concept iteration because every unit change needed to simultaneously be broken up by 2 week sprint AND be completely unit tested and flawless before asking someone to review it or getting it merged. Meanwhile, I'd never know when someone updated their PR or Jira thread after discussing unless I manually checked or always kept the browser tab open.

So I'd take a full morning to review a PR, then respond to things on Jira, and if I'm lucky, write some code. Eventually the PR would be updated but I'd forget to check because there wasn't a good system in-place to manage those notifications, and when I did it'd be another massive change that I'd need to take quite a lot of time to check.

So I started using Draft PRs for bigger or more complex changes, and explicitly note what wasn't functioning or written yet, which would provide for lower pressure time to get early feedback instead of trying to complete everything up front, and updates to those PRs would be more incremental.

Re: Claim: the ideal PR is 50 lines long

#117

Post by johnnyanmac »

stephenorr wrote: post #80
woooooo wrote: post #62

Earlier quoted context omitted.

5???? Please tell us more. Did 6 line commits get rejected? How did anything get done?

Well, things did get done, but not the right things. The rule wasn't hard and fast, but you had to have a damn good reason why you wanted to add bigger commits. Protobuf definitions didn't count, so you could get away with doing longer commits for those. Tests were discouraged anyway ("Google just tests in production", we were told), so there wasn't a lot of that sort of thing.

>"Google just tests in production", we were told

well that sadly explains a ton of little quirks Ive seen in all sorts of google software over the years. Chromecasting functions is especially egrigious and it feels like connecting/disconnecting does a different thing each time.

Re: Claim: the ideal PR is 50 lines long

#118

Post by [deleted] »

bvrmn wrote: post #107
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.

[deleted]

Re: Claim: the ideal PR is 50 lines long

#120

Post by yellowapple »

johnfn wrote: post #68
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…

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…

I've found that if a PR is sufficiently small then reviewing it doesn't entail quite as severe of a context switch as reviewing a larger PR. Something about it being shorter means I can keep some of the coding context fresh in my head so I can switch back to it quicker (whereas for a longer PR I need to jettison that context in order to comprehend the whole PR).
Post reply on HN