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

In Praise of Stacked PRs

benjamincongdon.me

Post reply on HN
Print view View original

71–80 of 230 posts

Re: In Praise of Stacked PRs

#71

Post by jcpst »

dreamdeath wrote: post #40
Supermancho wrote: post #37

Earlier quoted context omitted.

> First, think about how difficult, and time-consuming, it will be for others to digest and review 2500 new lines of code that sprung from someone else's mind. There's a tradeoff to be made. Have a feature sooner or later. Review now quickly and more carefully later, or review carefully now. Part of what development teams do, is risk assessment. Put a feature flag on it, do a demo of the branch. If it looks good, do…

> First, think about how difficult, and time-consuming, it will be for others to digest and review 2500 new lines of code that sprung from someone else's mind. I haven’t ever worked at big corp so maybe this kind of thinking is actually valuable there. But in most startups in my experience this mindset is wrong. You literally won’t have a job tomorrow (because your company will fold) if you don’t ship value-generatin…

To offer an alternative perspective: I haven’t ever worked at a startup.

If someone wanted a PR review for 2500 lines, I would tell them no. I absolutely expect a developer to think twice when trying to make a change that large in one go.

When your corporation’s billion lines of code are the engine that helps generate $30,000 a minute, you don’t just say “meh, looks good to me.”

Furthermore, this is multiplied on my team, which is the framework team. A mistake in our auth middleware, http client, etc, could easily turn into a mistake on 50+ other teams.

Re: In Praise of Stacked PRs

#72

Post by mrkeen »

is0tope wrote: post #8

I've usually kept a rule that you should avoid stacking, and if you must only one level deep. The fact that you have to stack in the first place typically suggests that PRs aren't being merged fast enough. Stacking in my personal experience usually leads to merge conflict hell as changes and PR suggestions get merged underneath you.

> The fact that you have to stack in the first place typically suggests that PRs aren't being merged fast enough

Yes. Whenever you inspect a proposed solution, you should hopefully find the problem that it solves.

Re: In Praise of Stacked PRs

#73

Post by californical »

chrisseaton wrote: post #56
is0tope wrote: post #8

I've usually kept a rule that you should avoid stacking, and if you must only one level deep. The fact that you have to stack in the first place typically suggests that PRs aren't being merged fast enough. Stacking in my personal experience usually leads to merge conflict hell as changes and PR suggestions get merged underneath you.

> The fact that you have to stack in the first place typically suggests that PRs aren't being merged fast enough. Unless PRs are merged instantly, I'm always going to be waiting after one PR is opened, before I can work on the next, unless I stack, aren't I? Is your definition of 'fast enough' instantly? If not, how does this work?

You can branch off of a PR, but someone should review and merge your first PR before your second is ready to be up in a PR again.

Also, trying to make units of work so that they don’t need to overlap like that can be useful too

Re: In Praise of Stacked PRs

#74

Post by nightpool »

chrisseaton wrote: post #56
is0tope wrote: post #8

I've usually kept a rule that you should avoid stacking, and if you must only one level deep. The fact that you have to stack in the first place typically suggests that PRs aren't being merged fast enough. Stacking in my personal experience usually leads to merge conflict hell as changes and PR suggestions get merged underneath you.

> The fact that you have to stack in the first place typically suggests that PRs aren't being merged fast enough. Unless PRs are merged instantly, I'm always going to be waiting after one PR is opened, before I can work on the next, unless I stack, aren't I? Is your definition of 'fast enough' instantly? If not, how does this work?

This is why I think it's really really important that all PR reviews be synchronous, so that there's never any time spent twiddling your thumbs or context switching onto another change. Also it just makes it much easier to review a PR when you can sit down and actually talk about it in real time with the author, rather than having the ping messages back and forth interminably until you reach an agreement

Re: In Praise of Stacked PRs

#75

Post by wpietri »

is0tope wrote: post #8

I've usually kept a rule that you should avoid stacking, and if you must only one level deep. The fact that you have to stack in the first place typically suggests that PRs aren't being merged fast enough. Stacking in my personal experience usually leads to merge conflict hell as changes and PR suggestions get merged underneath you.

For sure. One of the things I learned from the Lean folks was to look for inventory; it's one of the 7 Wastes. [1] In physical manufacturing, it's pretty obvious, because it's physical stuff sitting around on the journey to becoming actually useful.

With software it can be harder to notice because you don't have to make room for it. But in essence it's the same deal; it's anything we have paid to create that isn't yet delivering value to the people we serve. Plans, design docs, and any unshipped code.

There are a lot of reasons to avoid inventory, but a big one is that until something is in use, we haven't closed a feedback loop. Software inventory embodies untested hypotheses. E.g., a product manager thinks users will use X. A designer thinks Y will improve an interface for new users. A developer thinks Z will make for cleaner, faster code.

Both large pull requests and stacked pull requests increase inventory. In the case of incorrect hypotheses, they also increase rework. I could believe that for a well-performing team stacked PRs are better than equally-sized single big PRs, in that they could reduce inventory and cycle time. But like you, I think I'd rather just go for frequent, unstacked, small PRs.

[1] e.g. https://kanbanize.com/lean-management/value-waste/7-wastes-o...

Re: In Praise of Stacked PRs

#76

Post by celtain »

computronus wrote: post #30
ramraj07 wrote: post #12

Earlier quoted context omitted.

It’s unavoidable sometimes. I get inspiration and time together rarely, I can’t wait for small chunks of code to be merged before I continue. A lot of times it’s an extremely Productive Sunday afternoon and I have 2500 new lines of code that builds a full new prototype. What am I to do?

First, think about how difficult, and time-consuming, it will be for others to digest and review 2500 new lines of code that sprung from someone else's mind. So you will end up waiting anyway, for even a small part of your work to be merged. The work of breaking up a big, inspired chunk of work into small pieces helps you learn more about it, and the perspective can reveal improvements that weren't obvious in the ini…

Isn't this exactly why you should use stacked PRs? That's what it looks like when I break a big piece of work into smaller pieces.

What's the alternative anyway? If you don't want me writing 2500 lines of code in one area, would you rather I write 10 250 line PRs in 10 different parts of the codebase instead? Is that supposed to be easier to review?

Or is the rule just "don't write a lot of code in any short period of time"?

Re: In Praise of Stacked PRs

#77

Post by wpietri »

chrisseaton wrote: post #56
is0tope wrote: post #8

I've usually kept a rule that you should avoid stacking, and if you must only one level deep. The fact that you have to stack in the first place typically suggests that PRs aren't being merged fast enough. Stacking in my personal experience usually leads to merge conflict hell as changes and PR suggestions get merged underneath you.

> The fact that you have to stack in the first place typically suggests that PRs aren't being merged fast enough. Unless PRs are merged instantly, I'm always going to be waiting after one PR is opened, before I can work on the next, unless I stack, aren't I? Is your definition of 'fast enough' instantly? If not, how does this work?

One way this can work is pair programming. Especially if you also practice frequent pair rotation, you can get plenty of eyeballs on code in a timely fashion. That's my preference.

Another way is that you next work on something different enough that it doesn't need to stack. E.g., reviewing pull requests.

Re: In Praise of Stacked PRs

#78

Post by wpietri »

ignormies wrote: post #17
is0tope wrote: post #8

I've usually kept a rule that you should avoid stacking, and if you must only one level deep. The fact that you have to stack in the first place typically suggests that PRs aren't being merged fast enough. Stacking in my personal experience usually leads to merge conflict hell as changes and PR suggestions get merged underneath you.

Something I often use stacked diffs for is deprecation -> removal flows. 1. Deprecate old feature + add opt-in support for replacement 2. Make replacement default with opt-out for old pattern 3. Completely remove old feature and the opt-out functionality I can write the entire stack of diffs upfront, have them individually reviewed but still linked, and ensure they're merged in the correct order. The bottleneck for m…

Ooh, good point. I'm suspicious of stacked diffs generally, but this seems like a good example where we truly don't expect to learn anything after the initial deploy.

Re: In Praise of Stacked PRs

#79

Post by perrygeo »

chrisseaton wrote: post #56
is0tope wrote: post #8

I've usually kept a rule that you should avoid stacking, and if you must only one level deep. The fact that you have to stack in the first place typically suggests that PRs aren't being merged fast enough. Stacking in my personal experience usually leads to merge conflict hell as changes and PR suggestions get merged underneath you.

> The fact that you have to stack in the first place typically suggests that PRs aren't being merged fast enough. Unless PRs are merged instantly, I'm always going to be waiting after one PR is opened, before I can work on the next, unless I stack, aren't I? Is your definition of 'fast enough' instantly? If not, how does this work?

By pinging people on slack and interrupting them for a review asap? Or context switching to an unrelated part of the code in parallel while you wait? Or reducing the frequency of reviews and make a mega-PR every couple weeks?

I've seen no solutions, only tradeoffs but I'm curious if anyone has a tried and true way to avoid this traffic jam scenario.

Re: In Praise of Stacked PRs

#80

Post by zeroonetwothree »

rsstack wrote: post #3

We switched to using https://graphite.dev/ after yet-another huge epic that caused a mess of PRs and one PR that touched 100+ files and included dozens of semi-related changed. It's been a blessing so far.

Graphite is amazing! Would definitely recommend everyone try it at least :)
Post reply on HN