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

61–70 of 79 posts

Re: Large pull requests slow down development

#61

Post by tubthumper8 »

DarkPlayer wrote: post #37
hu3 wrote: post #25

Worst offender is when a dev decides to reformat code and you can't even differentiate at a glance what is real code change and what is reformatting. Fortunately modern tooling formats code automatically so there's less bikeshedding.

There are some tools that can separate actual code changes from reformatting changes. I am working on https://semanticdiff.com , a VS Code Extension / GitHub App that can help you with this. There is also difftastic if you prefer a CLI based solution. It supports more languages but can detect fewer types of reformatting changes.

I like this idea of advancing beyond line-based diffs into something more meaningful. If someone renames a variable/type/module/whatever which changes X lines of code across Y files, I really don't care to see all of that, for what I'm concerned with that's a single diff.

(Assuming the code is in a language with a good type system and without reflection, I'm confident in automatic refactors and don't need to spend time on them in code review)

Re: Large pull requests slow down development

#62

Post by schreiaj »

CuriousCosmic wrote: post #60
strangesmells06 wrote: post #46

Earlier quoted context omitted.

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.

And tools that facilitate and encourage that behavior.

I’ve tried to do this but the defaults on GH means even if I stack 5 commits each doing atomic things nobody reviews it as anything other than a single patch. And if I split them into distinct PRs the last one looks like the giant PR. And we’re in now I’m shepherding 5 PRs.

Until we have tools or at the very least processes that encourage that behavior nobody will do it.

And don’t get me started on commit message display.

Re: Large pull requests slow down development

#63

Post by eru »

jacobegold wrote: post #7
eru wrote: post #4

Earlier quoted context omitted.

> [...] but reality is there are a lot of smart devs who are "big thinker" types and struggle with incremental development and lots small PRs towards a bigger goal. It doesn't really matter. You can start with a big change initially as a 'big thinker'. You just have to break it down afterwards. I often have a bit of feature creep when working on a change, and add all kinds of incidental fixes I find along the way. Bu…

I like this take. The onus is on these devs to learn how to use available tools effectively to split up their changes. There's a clear parallel here to the idea of a "genius" who isn't able to communicate their ideas effectively. Can they really be considered that smart if no one understands them?

Yes, though it's is all about trade-offs. It works best, if your reviewers are also putting in some work.

Re: Large pull requests slow down development

#64

Post by withinboredom »

AlotOfReading wrote: post #49
HDThoreaun wrote: post #40

Earlier quoted context omitted.

I mean this is mostly just a devops issue though no? PR’s used to take a solid week to merge at my company with a 10 hour CI that had a ~50% success rate when the code worked. But we put a bunch of resources toward build times and ci run time and now it mostly works. Yea, you can circumvent a shitty pr system by just making massive pr’s but wouldn’t it be easier to have the devops team fix your broken system?

Depends on what your code does. A lot of my code runs robots. You can't scale horizontally because you can only fit so many systems in a room before you run afoul of production limits, safety requirements or cooling capacity. You can't scale vertically because zoning. You can't run the tests faster because physics. So, you either run the expensive tests asynchronously and all that entails or you allow CI to take fore…

In that case, would it make more sense to have a small PR that you start with, get it reviewed, then open smaller PRs to your original PR? That way your approach/code can be reviewed while in your larger PR you tweak and fix tests.

Re: Large pull requests slow down development

#65

Post by mewpmewp2 »

k_dumez wrote: post #15
sb8244 wrote: post #3

I believe that there's certain things (particularly ambitious net new feature development) that isn't served well by artificially minimizing PR size. You should absolutely strive to keep smaller PRs, but I've frequently seen this become "you should only have small PRs."

> artificially minimizing PR size Not sure I understand the "artificial" part here. There's nothing "artificial" about breaking up your larger changes into smaller PRs. It's just good practice. Helps reviewers who are reviewing the code, and helps the author be more focused with their changes. Even in net new feature development it's a good idea to break up your large changes to something more manageable. Sorry if I'…

To me in a lot of cases it seems even as a reviewer, it's harder to understand big picture if someone splits up the PRs.

But as a code writer myself, for example, if I am building a new feature, firstly it's really hard for me to know what the whole thing would look like without going through it all and it's probably very iterative process as I'm doing it, so I usually wouldn't be able to split it up or it would very suboptimal to split it up before I've finished everything.

Then I would try to split it up as I've finished to appease reviewers, but again, it requires whole lot of creativity to do. Should I try to split up shared component first? Because I surely can't split up whatever is using those shared components. If I do then, people won't see whatever is implementing those shared components so they won't have understanding on why those shared components provide certain functionality etc.

Overall it complicates a lot it seems because if I was to do it during my iterative process then I would write a PR, later refactor bunch of it anyway, and I would do it in the order that feels best for me, but wouldn't necessarily be easy to understand for anyone not within it.

Re: Large pull requests slow down development

#66

Post by mewpmewp2 »

CuriousCosmic wrote: post #33
sb8244 wrote: post #29

Earlier quoted context omitted.

It's not necessarily about the units of code, it's about the bigger picture. Let's give a hypothetical situation where you split up a PR by the backend and frontend components, but you have some extra fields and endpoints that are not used in your final product. They accidentally get left in because you miss them. I believe the chance that it would be caught in review is significantly higher in a unified PR review in…

> And if you need to have the 2 PRs open side by side, then why split it up in the first place? I agree however I think PR's should be split up. Just not into separate PRs. The solution is to actually start reviewing PRs by commit (you can do this in the PR web interface). Everything is split up and can be reviewed separately but you can still see the final diff and while each discrete unit is reviewable, the greater…

But if you are doing by commit and it later gets iteratively refactored, wouldn't that be a huge waste of time?

How would you know what to critically evaluate in such case?

Re: Large pull requests slow down development

#67

Post by mewpmewp2 »

chrnola wrote: post #26
nine_zeros wrote: post #13

There is one place where large pull requests are essential: In dysfunctional orgs with enormous code review time per PR, with lots of nitpicking, and a CI/CD pipeline that is constantly broken and takes a LONG time to get things into a staging environment.

Maybe I’m just hopelessly cynical, but I suspect that this is representative of the average developer’s experience.

In large corps it could be yes. In smaller and more nimble companies/start ups, there's a lot of good pipelines.

Re: Large pull requests slow down development

#68

Post by mewpmewp2 »

eru wrote: post #4
gedy wrote: post #2

I agree, but reality is there are a lot of smart devs who are "big thinker" types and struggle with incremental development and lots small PRs towards a bigger goal. I prefer incremental and used to roll my eyes at them, but it's just a different way of thinking and people/teams are diverse.

> [...] but reality is there are a lot of smart devs who are "big thinker" types and struggle with incremental development and lots small PRs towards a bigger goal. It doesn't really matter. You can start with a big change initially as a 'big thinker'. You just have to break it down afterwards. I often have a bit of feature creep when working on a change, and add all kinds of incidental fixes I find along the way. Bu…

> It doesn't really matter. You can start with a big change initially as a 'big thinker'. You just have to break it down afterwards.

But that's extremely difficult I think and requires some out of the box extreme creative thinking on how to split it up after the fact. And I would also think it doesn't help at all.

I usually like to imagine how I build the new feature in head. Then I vomit out whole bunch of code, but I feel like splitting it up is extremely difficult and it will just lose context of why I did something as I did.

E.g. if I split it in a way where I only show shared components, it won't be understandable why I made them shared in certain way because you won't see the actual other logic that uses them, so reviewers are kind of left to trust that these will be used in later PRs.

And I think the way you make things reusable is likely the most important part because there's a fine balance there, you don't want to make everything too reusable that is not intended to be reusable, but at the same time you want to make it as reusable as possible considering the future, and it will take a lot of thinking to understand future implications. A reviewer won't have understanding of the reasoning without going through the process themselves on how something might be reused.

There's a lot of disagreement as well on how DRY you exactly have to be. Some people follow the rule of write it 3 times then refactor to be reusable, some want to do it immediately, etc. And having shared components first no one knows what is going on.

And then if they were to criticise my shared logic, and they want me to change anything, which could even be a change they request because of misunderstanding I will have to change all the other logic down the other PRs as well.

Re: Large pull requests slow down development

#69

Post by CuriousCosmic »

mewpmewp2 wrote: post #66
CuriousCosmic wrote: post #33

Earlier quoted context omitted.

> And if you need to have the 2 PRs open side by side, then why split it up in the first place? I agree however I think PR's should be split up. Just not into separate PRs. The solution is to actually start reviewing PRs by commit (you can do this in the PR web interface). Everything is split up and can be reviewed separately but you can still see the final diff and while each discrete unit is reviewable, the greater…

But if you are doing by commit and it later gets iteratively refactored, wouldn't that be a huge waste of time? How would you know what to critically evaluate in such case?

I'm confused what you mean?

If you are talking about refactoring prior to merging into the tree? then no it's not a waste of time. That's the intended workflow. You make your changes to the commits or add new commits in between using rebase. This is how all the development for linux is done.

If you are talking about after they are merged into the mainline? That's also not a waste of time. You don't have to go back and change those commits because they are now finalised and your iterative refactoring can be done via small one off patches gradually merged into main, a series of large patches merged into main, or a set of patch series all merged into an incubation branch that is kept up with main and eventually merged back into main once the refactor has made sufficient progress to take over.

Re: Large pull requests slow down development

#70

Post by mock-possum »

sb8244 wrote: post #36
CuriousCosmic wrote: post #33

Earlier quoted context omitted.

> And if you need to have the 2 PRs open side by side, then why split it up in the first place? I agree however I think PR's should be split up. Just not into separate PRs. The solution is to actually start reviewing PRs by commit (you can do this in the PR web interface). Everything is split up and can be reviewed separately but you can still see the final diff and while each discrete unit is reviewable, the greater…

I agree big time with this. Organizing by clean commits is definitely important.

Why’s that? Do you step through commits diff by diff when reviewing?
Post reply on HN