Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Large pull requests slow down development

graphite.dev

Post reply on HN
Print view View original

51–60 of 79 posts

Re: Large pull requests slow down development

#51

Post by mvdtnz »

ckdarby wrote: post #18

I'm writing a book, "Bytes Of Wisdom: From Start To Merged" that focuses around code reviews. I agree that larger PRs can increase the latency of getting feedback and getting quality feedback. I also have another view. Most code reviews struggle not from the reviewing part and shockingly not the authoring part, but the laying groundwork part. The laying the groundwork part is before any hands to the keyboard to write…

I agree with you for significant changes. But most day to day programmers are working on large and/or mature codebases, and I would think that this level of groundwork should not be required for a typical change. Indeed I would hope I don't need to bring multiple stakeholders along on the journey in order to add a couple of columns to a report or upgrade an API schema version.

Re: Large pull requests slow down development

#52

Post by mvdtnz »

gavmor wrote: post #38
ckdarby wrote: post #18

I'm writing a book, "Bytes Of Wisdom: From Start To Merged" that focuses around code reviews. I agree that larger PRs can increase the latency of getting feedback and getting quality feedback. I also have another view. Most code reviews struggle not from the reviewing part and shockingly not the authoring part, but the laying groundwork part. The laying the groundwork part is before any hands to the keyboard to write…

Well that sounds like a good read! I've been privileged to spend my 10+ years writing software among pair-programmers, and have never been made to suffer from this "code review" ritual. Maybe it's not so bad? I'd love to read a book about it before my luck runs out and somebody foists it upon me. Just to be prepared, you know. Seems to me like it just can't work. Show a programmer a 5 line program, as the adage goes,…

Code review is not a "ritual". Our code is our output, and it is paid work. Most professions have some level of review. Go speak to a chartered accountant or an engineer and complain about the "ritual" of peer review and see how far you get.

We are professionals and it's about time we started acting like it.

Re: Large pull requests slow down development

#53

Post by indymike »

It's intuitive to look at size (as in lines changed) as a complexity metric. What is interesting about this article is that they measured what effect the number of files changed in the PR had on review and time to merge. The more files changed, the slower the review... If there were too many files changed, then the reviewer would spend less time looking at the changes before approving.

Re: Large pull requests slow down development

#54

Post by mvdtnz »

CuriousCosmic wrote: post #21

I suppose I agree but I don't think it's really accurate. You can make large, easy to review PRs but if the commit history isn't useful then you have to view it all in one big chunk. If instead you take a note out of the patchset workflow and make each commit it's own fairly discrete and digestible change after you wrap up development on the feature, then reviewers can just start on commit 1/N and review each one as…

I am not exaggerating when I say that if I had the power to do so, if you gave me a 10,000 line PR I would fire you on the spot. As far as I'm concerned that is professional negligence.

(Obviously I make exceptions for things like large scale code generation or automated refractors).

Re: Large pull requests slow down development

#55

Post by mvdtnz »

ridiculous_fish wrote: post #48

This article implicitly assumes that a single PR corresponds to a single commit. It doesn't touch on the idea of multiple commits in a single PR. Consider adding a feature. I might structure this as three commits: 1. Refactor in a way that preserves existing behavior but prepares for the new feature 2. Implement the feature, creating a user-visible change 3. Remove old code rendered unnecessary with the new feature E…

> This article implicitly assumes that a single PR corresponds to a single commit.

It doesn't?

As for the rest of your comment, this is why we have feature flags. What you're describing should be a single PR. I don't care how many commits it is.

Re: Large pull requests slow down development

#57

Post by [deleted] »

mvdtnz wrote: post #52
gavmor wrote: post #38

Earlier quoted context omitted.

Well that sounds like a good read! I've been privileged to spend my 10+ years writing software among pair-programmers, and have never been made to suffer from this "code review" ritual. Maybe it's not so bad? I'd love to read a book about it before my luck runs out and somebody foists it upon me. Just to be prepared, you know. Seems to me like it just can't work. Show a programmer a 5 line program, as the adage goes,…

Code review is not a "ritual". Our code is our output, and it is paid work. Most professions have some level of review. Go speak to a chartered accountant or an engineer and complain about the "ritual" of peer review and see how far you get. We are professionals and it's about time we started acting like it.

[deleted]

Re: Large pull requests slow down development

#58

Post by yjftsjthsd-h »

steelframe wrote: post #30

Where I work it takes several hours to get a PR past all the merge gates. If I have 5 small PRs that all build one on top of the other, it can take me the better part of 2 weeks to get them all merged, counting all the context-switching, tweaking based on comments and relaunching of the merge checks, etc. The only way developers are able to make any kind of velocity in their work is to babysit their PRs morning, even…

Well okay, yes, if your process is that broken then you have to make do. In shops with a vaguely reasonable path to merge a PR, time for human reviewers to sign off dominates and smaller changes are the expedient path to get changes merged quickly.

Re: Large pull requests slow down development

#59

Post by CuriousCosmic »

mvdtnz wrote: post #54
CuriousCosmic wrote: post #21

I suppose I agree but I don't think it's really accurate. You can make large, easy to review PRs but if the commit history isn't useful then you have to view it all in one big chunk. If instead you take a note out of the patchset workflow and make each commit it's own fairly discrete and digestible change after you wrap up development on the feature, then reviewers can just start on commit 1/N and review each one as…

I am not exaggerating when I say that if I had the power to do so, if you gave me a 10,000 line PR I would fire you on the spot. As far as I'm concerned that is professional negligence. (Obviously I make exceptions for things like large scale code generation or automated refractors).

Yeah I'd agree if there hasn't been prior review and it's not new code. I normally would set my hard limit around 1-2k LOC and I'm expecting quite a few commits to break that up into something digestible or else it's an instant reject.

But for new code? I'm okay with more LOC than normal but I think 10k lines is my hard limit (including tests and documentation). And I expect no more than 500 lines in any given commit/patch. And realistically I am expecting that patchset/PR to be around 50-100 discrete commits/patches in length. Also it's likely that once things start stabilising that patchset can be broken up and merged in chunk by chunk.

And with prior reviews (i.e. merging an incubation branch into mainline) I expect there to be effectively no changes other than merge resolutions to aggregate PRs/patchsets into the incubation branch that is getting pulled into mainline.

Re: Large pull requests slow down development

#60

Post by CuriousCosmic »

strangesmells06 wrote: post #46
CuriousCosmic wrote: post #21

I suppose I agree but I don't think it's really accurate. You can make large, easy to review PRs but if the commit history isn't useful then you have to view it all in one big chunk. If instead you take a note out of the patchset workflow and make each commit it's own fairly discrete and digestible change after you wrap up development on the feature, then reviewers can just start on commit 1/N and review each one as…

Most people are not that good at git from my experience to be able to do this.

I agree but only because we don't expect people to be.

We expect clean, readable code so we develop engineers who can write that code.

If we expect clean, readable commit histories (like any project on the lore. https://lore.kernel.org/), we can develop engineers who produce those commit histories.

There's an adoption cost but IMHO it's worth it.

Post reply on HN