DEV Community

Discussion on: Do Stacked-PRs require re-review after merge?

Collapse
 
jlouzado profile image
Joel Louzado

I understand you don't use a development branch, and push straight to master

I feel like this problem would occur even if there was a develop or other intermediate branch, but I take your point that the fact that this is going into master raises the stakes somewhat.

...essentially making it difficult to make real changes (if some would come up in PR-1).

This is true, changes in PR-1 would need to be propagated to the "downstream" branches manually.

If you insist on going about your way...

So I'm not really married to this process, I was drawn to it because it makes the review-experience better for the rest of the team because I've done this additional planning to chunk up a big task into cognitively smaller pieces.

In my opinion the PR must be revalidated (maybe more briefly), and must pass your test suite again.

Assume that TDD is followed, and test-suite was passed for both PR-1, PR-2 and (PR-1 + PR-2). How would that impact your confidence in (PR-1 + PR-2)?

...and then branching off from master for feat/part2.

In an ideal case I would prefer this, but sometimes there's pressure on time and I might not have the luxury to wait for part1 to be merged before beginning work on part2.

Sometimes part1 might just be a refactor, and part2 is the actual feature in which case it becomes even more important to have a workflow that optimizes for throughput.

Collapse
 
dorshinar profile image
Dor Shinar • Edited

This is true, changes in PR-1 would need to be propagated to the "downstream" branches manually.

Sometimes part1 might just be a refactor, and part2 is the actual feature...

I've combined both statements because to me, they are both supportive of merging to master (or another shared branch). If you are working on a refactor, I'm sure other developers on your team would love to have it merged sooner, so that they may start using it.

...I was drawn to it because it makes the review-experience better for the rest of the team because I've done this additional planning to chunk up a big task into cognitively smaller pieces.

I'm all for chunking up big tasks - it is really difficult to give a constructive review when you look at a diff that contains dozens of files.

In an ideal case I would prefer this, but sometimes there's pressure on time...

Well, then I think you should talk to your team leader about it, and find a way to justify it. In my opinion, it might shorten the review period if all goes well, but if not the price will be higher (again - IMO).

Assume that TDD is followed, and test-suite was passed for both PR-1, PR-2 and (PR-1 + PR-2). How would that impact your confidence in (PR-1 + PR-2)?

In that case it a much safer merge. I would still give it a quick look before merging, but it's passable.

Thread Thread
 
jlouzado profile image
Joel Louzado

In that case it a much safer merge.

Yes, it would definitely be safer if TDD was followed but I was hoping there would be a general way to answer the question: Is the sum of two approved PRs also an approved PR?

The answer is probably some function of tests, thoroughness of review process and a few other factors (perhaps). Thoughts?

Thread Thread
 
dorshinar profile image
Dor Shinar

Like much of what we do, there isn't a single right answer here. It depends on your confidence in your code and your tests.
My general answer would be no -
PR-1 + PR-2 =/> PR-1 * PR-2 (sum meaning both approved, multiplication meaning one merged into the other).
But, that does not mean that under certain conditions (which I believe were met in your case), it can't be assumed.