Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

General question on PR's, when several PR's have a dependency on a commone one #9480

Closed
abelbraaksma opened this issue Jun 17, 2020 · 5 comments

Comments

@abelbraaksma
Copy link
Contributor

abelbraaksma commented Jun 17, 2020

Not a bug, just a question on expected best practice in this repo. Suppose you prepared a few PR's, but some of these are dependent on a different change (in my case: I just need to move a function up in a module so it gets in scope for others), how best go about it? I can think of:

  1. Create a separate branch + PR for the common change, then start new branches off this branch for the next PR's.
    • Downside: each dependent PR will show the changes of the common PR which may confuse reviewers
    • Upside: I can submit all PR's without necessarily waiting for agreement
  2. Create a separate branch + PR for the common change. Wait until this is merged into master, then start other PR's off this branch.
    • Downside: you'll need to wait before the common PR is accepted. If you continue with new PR's locally, you'll have to rebase.
    • Upside: cleaner history, easier review as each PR is isolated
  3. If it's a small common change, do that change for each PR and let merging fix it.
    • Downside: each PR will have this change, merging may show conflicts, reviewers may be confused seeing the same change
    • Upside: no added PR-dependency complexity

I'm sure this stuff happens all the time and I'm not Git-whiz, so perhaps / hopefully there's a simple(r) answer here.

(I'm currently assuming #1 or #2 to be the proper way, which I started #9481)

@smoothdeveloper
Copy link
Contributor

I've used 1 myself for #8352 which is depended upon in #8354, 8352 was useful on it's own to add a new test.

If the base PR doesn't have use in itself, I think only doing the "leaf" PRs off a common branch would be the approach.

I don't think option 1 is confusing for reviewer as it contains all the changes that are to be reviewed against master, in case the base PR gets merged, the diff automatically disappear (assuming they are the same commits).

@abelbraaksma
Copy link
Contributor Author

I don't think option 1 is confusing for reviewer as it contains all the changes that are to be reviewed against master, in case the base PR gets merged, the diff automatically disappear (assuming they are the same commits).

@smoothdeveloper, tx! Yes, that's the idea. I didn't think of the disappearing diff after merge-into-master, that makes sense. I already started this way, glad it is the right approach (now just re-do the PR where I didn't do it this way).

@NinoFloris
Copy link
Contributor

Thanks for tagging in FSSF

If you know it's going to be in quickly I'd wait, otherwise rebase on the common change and clearly communicate up to which commit the common change goes, github doesn't really facilitate here sadly

@cartermp
Copy link
Contributor

For this PR, pre-work changes that are functionally equivalent are perfectly fine to create a PR for. So long as the dependent PRs state the pre-work as a prerequisite, we can handle that.

@abelbraaksma
Copy link
Contributor Author

@cartermp & @NinoFloris, ok, that's how I did it now, with a line in the PR pointing at the dependency. Thanks @ALL!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants