Merging of branches currently in review

[jsirevag]

Consider the following scenario:

A PR is opened for feature/number-one, and while that is being reviewed, the efficient go-getting developer starts working on a new feature. The new feature builds upon some of the same files/changes as the branch already in review, so feature/number-two is then branched out of feature/number-one.

When feature/number-two is ready for a PR, should it be requested merged into develop or feature/number-one?

If it goes into develop, should feature/number-two have a requirement that feature/number-one be merged first? (As to not merge code that is being requested merged in another PR)

And how do we handle changes in feature/number-one during review, after feature/number-two branch was made or submitted for PR?

We might want a convention for this, and add it to the playbook.

[fespinoza]

This is my opinion

When feature/number-two is ready for a PR, should it be requested merged into develop or feature/number-one?

I personally would prefer develop, if the second PR depends on the first, i would try to merge the first one. In the other scenario, you merge PR #2, and then you get a big ass PR #1, which would lack the details of PR #2 (comments on the PR)

If it goes into develop, should feature/number-two have a requirement that feature/number-one be merged first? (As to not merge code that is being requested merged in another PR)

here is an example of this

And how do we handle changes in feature/number-one during review, after feature/number-two branch was made or submitted for PR?

after changing and merging PR #1, i would rebase the second branch to develop or if it's not merged yet, but with changes, i would rebase to feature/number-two

the details of this approach, is when you merge feature/number-one, is that the list of changes of feature/number-two will still include the list of changes of feature/number-one, and there is no posibility (that i know of) to refresh it, I asked for this feature to github, but it's difficult to know if they will do it and when.

[3lvis]

@fespinoza myshop page not found

[fespinoza]

you don't have access to the repo @3lvis but the rest of hyper do, i guess i should not mention that example though :P

[3lvis]

@fespinoza That was my point :stuck_out_tongue:

Btw, I like what you did there.

[fespinoza]

[sindrenm]

This is a tricky one, guys. Considering that feature/number-two is based off of an unmerged feature/number-one (as per your example, @jsirevag), then, if pointing feature/number-two towards develop, it would render the whole feature/number-one PR useless, as by reviewing feature/number-two, contents from feature/number-one is included in the process.

Your argument, @fespinoza, about feature/number-one becoming massive if feature/number-two is merged into it, is sort of irrelephant, because the same applies to feature/number-two if pointed towards develop. It only grows shorter after feature/number-one is merged.

I'd definitely prefer as much as possible to refrain from making PRs that are built upon other PRs. Instead, trying to build features isolated from each other sounds like a better solution to me. In some cases this is harder than others, though. For instance, if one PR sets up the whole front-end layout for a web application, and other PRs create individual pages, then those page PRs may want to utilize Sass mixins or container classes or whatever from the layout PR, but I'd try my best to separate them as much as possible.

[sindrenm]

So, essentially, what I kinda failed to mention explicitly, but tried to bring to the table, is that really, a PR shouldn't base itself off of another PR. So per the example branches again, I think both feature/number-one and feature/number-two should branch out from develop. :smiley:

EDIT: I see now that I didn't fail to mention this. But it doesn't hurt to mention it again anyway. :grinning:

[jgorset]

Wait - if you have a branch feature/number-one and that is merged into develop, merging feature/number-two into the now merged (and probably even deleted) feature/number-one does not much. It will never be part of develop, because its parent was already merged. No?

[fespinoza]

So, essentially, what I kinda failed to mention explicitly, but tried to bring to the table, is that really, a PR shouldn't base itself off of another PR. So per the example branches again, I think both feature/number-one and feature/number-two should branch out from develop

that is true, that is the ideal case, but in many ways that does not happen on day to day work, let's say i am working on a project and in a PR i create the user models, and I need to work on permissions, i need the users to work on permissions, so meanwhile the PR is being reviewed for users I can work on the permissions in a separate branch

Your argument, @fespinoza, about feature/number-one becoming massive if feature/number-two is merged into it, is sort of irrelephant, because the same applies to feature/number-two if pointed towards develop. It only grows shorter after feature/number-one is merged.

there will be a massive PR, but i prefer that to be the second one, so if you have PR A and B, where B starts from A, I prefer to have a PR not that massive for A and then a massive PR for A+B, because I can accept only A and maybe not A+B. I have that option

[sindrenm]

@jgorset: Yeah, if feature/number-one is merged before feature/number-two, then feature/number-two will need to point towards develop. Which is another reason why I think feature branches should not want to get merged into other feature branches, they should be separate. Both feature/number-one and feature/number-two should get merged directly to develop.

@fespinoza: What good is your User model feature if another developer reviews and merges your permission PR before even noticing there's a separate PR for the User model?

[jgorset]

We forgot about this one. I made an attempt to summarize our discussion and conclusion in #45 – what do you guys think?