-
Notifications
You must be signed in to change notification settings - Fork 800
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
Comments
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). |
@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). |
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 |
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. |
@cartermp & @NinoFloris, ok, that's how I did it now, with a line in the PR pointing at the dependency. Thanks @ALL! |
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:
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)The text was updated successfully, but these errors were encountered: