[chrisseaton]

> 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?

[majormajor]

Two top-level scenarios here:

* you're working on the same part of the code

* you aren't working on the same part of the code

The second scenario is common but also trivial here since you can just have parallel branches going, so I'm gonna assume more the first - working on something that's building on top of what you just put up for review.

Let's say the review is done in 2 hours. If you're already done with the followup, IMO you may be erring too far on the side of "small PRs." If you aren't, you just rebase on top of whatever changes had to be made, if any, to the first one, once it lands on the primary branch.

If, on the other hand, the review isn't done for 2 days... then that's a PR turnaround time problem for sure.

But I strongly disagree with the people saying "multiple dependent PRs suggests the work wasn't split up properly" - there's nothing worse than a mega-PR of thousands of lines for the sake of doing a "single feature" all in one shot vs having to possibly pause and rebase periodically after review. It's even more painful when this mega-PR requires fundamental changes that could've been caught earlier, but now will take longer to adjust, and then will stay open for a while and likely result in merge conflicts as a result.

[atq2119]

> Let's say the review is done in 2 hours.

One of the projects I work on is a large, well-known open source project. Reviews that take days are pretty common.

> If you're already done with the followup, IMO you may be erring too far on the side of "small PRs."

There have been times when I created a sequence of three or four separate commits within under half an hour.

This was on projects where test coverage is tricky (think hardware interfaces) and keeping changes small was motivated a lot by better bisectability.

Clearly, software development is a pretty broad field with lots of different experiences.

[perrygeo]

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.

[wowokay]

Yes, complete your story in a single PR, if the next story is related and requires your changes your work wasn’t split up properly, plus in that scenario there is a good chance other devs have to many dependencies on other devs work which is a recipe for disaster!

[nightpool]

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

[wizofaus]

In one of my first jobs, code reviews were done exactly like this, with the the reviewer (my boss) and I sitting together at one computer going through the changes. It definitely has benefits but it's still important to ensure recommendations/concerns etc. get written down and doesn't necessarily help if there's a need to go away and make significant changes based on the outcomes of discussions. But while the back & forth discussion that often occurs during review can benefit from being synchronous, it might still be some time before your coworker has available time to participate in such a session, so it's still likely you'll need something else to work on in the mean time.

[nightpool]

> and doesn't necessarily help if there's a need to go away and make significant changes based on the outcomes of discussions.

See, I disagree, because this is absolutely the place where it helps the most—you can now go away and keep working on the same task without having to context switch to anything else or remember where you were or what you were working on. So you're never in a state where you're blocked and can't work on your ticket, and you don't have anything else to do or have to pick up another ticket and start learning about a whole completely separate problem. (And yes, definitely everything still needs to be written down, it's important to walk away knowing which changes you need to make, and why!)

> it might still be some time before your coworker has available time to participate in such a session, so it's still likely you'll need something else to work on in the mean time.

In teams I've worked on, the expectation is that an engineers highest priority is always unblocking another engineer, so this very rarely happened. Unless they had an interview to go to or some kind of meeting—and in that case, you could always just ping somebody else.

Obviously it's a style of work that relies really heavily on everyone sharing the same timezone and work hours, but it works really well to eliminate time lost to context switching and minimize the amount of time engineers spend blocked waiting on someone else.

[wizofaus]

But if you're expecting another dev to always be available to help you with a review, then they're the ones having to "context-switch", interrupting their own work. The reality is context-switching is something that comes with the territory if you're working as part of a team developing software. Which isn't to say there aren't opportunities to minimise the disruption it causes, but the idea that you can more or less eliminate it seems like wishful thinking. Perhaps there's a case for minimising it for more junior devs that aren't as capable of dealing with it, though it's equally possible younger brains are better at it!

[nightpool]

I think maybe we're using different definitions of the term "context-switch". Certainly it's an interruption, but I don't really think that sitting down with another engineer to do a focused code review where they've already written out a PR description and thought hard about the problem is comparable to starting a brand new branch or picking it up after a while away and trying to juggle 2, 3, or 4 in-progress tickets.

[wizofaus]

It requires a mental switch in focus. For me anyway, it's not a big difference if it's between working on implementing feature A then switching to working on implementing feature B, and working on feature A then switching to doing a code review for feature B, but I probably take my code reviewing responsibilities more seriously than most (e.g. I always checkout the branch locally, ensure it builds and runs etc.).

[daniel-grigg]

This sounds like you’re just externalising the costs of integration of your changes on to other people.

Generally we want to reduce any accidentally complexity and the way to achieve that is to reduce the latency of code reaching production. So for example you want to minimise PR/branches in flight ideally to 0. So you should be asking how you can reduce them - eg trunk based development, omitting asynchronous code reviews etc.

[nightpool]

> This sounds like you’re just externalising the costs of integration of your changes on to other people.

Huh? Nothing I said has anything to do with "the cost of integrating changes". I'm saying that it's more efficient for developers to work on one single task at once, instead of trying to "multi-task" among a variety of unrelated changes, and that synchronous code reviews help with this because they minimize the amount of time a developer spends blocked waiting for feedback from the rest of the team. Certainly this also has the benefit that you get fewer merge conflicts and have fewer branches in flight, but I don't know why you're saying that synchronous code reviews are "just externalizing the costs of integration [...] on to other people". That doesn't make any sense.

[delecti]

If your second PR is ready before the first PR is merged, then two of the likliest outcomes are that either PR reviews are taking too long, or the second PR is small enough that it could have just been part of the first. Alternatively, the review is taking a long time because the first PR was bad/controversial, in which case the assumptions of the second PR might need to be reevaluated anyway.

[dairylee]

Neither of those cases need to be true for a second PR to be ready before the first has been merged.

For example you do your first PR, mark it ready for review. While doing it you notice there's some refactoring you could do to some tangentially related code. It's very conceivable that the second refactoring PR could be ready pretty quickly.

[delecti]

There's a reason I said "two of the likliest outcomes", and not "the only two possibilities".

Yes there are other situations, but in my experience, the ones I mentioned are the most common.

[californical]

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

[wowokay]

No? You can create a new branch and start working on the next thing. Why would you be waiting on your PR to complete unless you didn’t split your work correctly.

[chrisseaton]

What if the next thing depends on the previous thing?

[fuzzy2]

Then your new branch starts at the tip of the previous one.

You can (/will have to) rebase later.

[chrisseaton]

I thought that was what stacked PRs are - maybe not?

[wpietri]

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.