-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Add branch tracking to development workflow instructions #28575
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
Add branch tracking to development workflow instructions #28575
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
Thanks for the PR! I believe, the current form is historic and should be reconsidered fundamentally. git 1.7 was released in 2010. Do we really have to make things more complicated by distinguisheing and explaining workflows for software that is 14 years old? I'd rather just assume that everybody has a newer version. But irrespectively, I would not recommend
I propose to just delete this block: Note also that https://learn.scientific-python.org/contributors/first-contribution/ does not mention Additionally, I propose to put the following paragraph into a |
To be fair to @juanis2112, she said the same thing and I was the one who said to put it in just in case there was someone who had very strong opinions about us keeping it. If there aren't any, then 🤷♀️ I don't have an issue with deleting it. |
Thanks for your comments @timhoffm. I was actually the one who wrote the scientific python guide and now I'm reconsidering some stuff there based on the experience I've had at the SciPy sprints. I will probably review it and make some edits after the sprints. I agree there's no need to add workflow explanations for old versions of git so I'll go ahead and remove the tab. About tracking the branches, I added the 'set-upstream' part because that's what git suggests when you only do 'git push' but it does require further understanding. It's definitely not needed for doing the 'first PR' but if there's extra changes needed within the same branch, this is something that people will eventually have to do so maybe it is good to have it somewhere after. For example in the sprints guide it is mentioned later. In the Matplotlib guide it probably belongs to the section 'Update a pull request'. Let me know if this makes sense and I'll do it that way. Also I'm happy to implement the hint too. |
I’m confused by this as I always just do |
🚀 I would find a diagram of the workflow helpful. There are many visualization variants out there. I likte this one, tough I would make a stronger visual connection between the the two occurences (main + feature branch) of the local and of the remote repos. |
Oh, I get it now. Thanks! |
It looks like some exception handling changes accidentally made it into this PR branch? |
Those are on |
@@ -0,0 +1,26 @@ | |||
Exception handling control |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something does not look right with the git history here.
Thanks for your suggestions @timhoffm. I just made a commit with the changes. I'll also look into the diagram idea once I start working on the SP guides. Not sure what the 'Exeption handling control' changes are. I did do a rebase on this PR after opening since there were some changes pushed to main while I was working on it. @tacaswell please let me know what to do or how to fix this. |
Could you have accidentally pulled main into your feature branch when rebasing? That exception came in from #28573 and another rebase should fix it. |
54df556
to
315fbb3
Compare
Thanks @QuLogic. I followed the steps, including squashing the commits and updating the commit message. However I don't see the new commit message reflected. Also, the circle ci tests are still failing but it doesn't seem related to any files I changed. Thoughts? |
Since it looks like the actual commit (at least on here) is fine, you can try |
Ah sorry, in the instructions quoted above, I assumed your
Or you can rebase against the upstream branch directly:
As part of the rebase, you can change |
315fbb3
to
b488524
Compare
Delete tab for branch tracking workflow Apply suggestions from code review Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
b488524
to
9961c6f
Compare
doc/devel/development_workflow.rst
Outdated
@@ -167,6 +161,17 @@ You can achieve this by using | |||
git commit -a --amend --no-edit | |||
git push [your-remote-repo] [your-branch] --force-with-lease | |||
|
|||
.. tip:: | |||
Instead of typying your branch name every time, you can once do:: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of typying your branch name every time, you can once do:: | |
Instead of typing your branch name every time, you can once do:: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"you can once do" is transposed construction, should be "you can do once" but really something like "instead of typing your branch name every time, you only need to type the following once to link the remote branch to the local branch"
``--set-upstream`` option:: | ||
.. hint:: | ||
|
||
git push --set-upstream origin my-new-feature | ||
|
||
From now on git will know that ``my-new-feature`` is related to the | ||
``my-new-feature`` branch in the GitHub repo. | ||
|
||
If you first opened the pull request from your ``main`` branch and then | ||
converted it to a feature branch, you will need to close the original pull | ||
request and open a new pull request from the renamed branch. See | ||
`GitHub: working with branches | ||
<https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-branches#working-with-branches>`_. | ||
If you first opened the pull request from your ``main`` branch and then | ||
converted it to a feature branch, you will need to close the original pull | ||
request and open a new pull request from the renamed branch. See | ||
`GitHub: working with branches | ||
<https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-branches#working-with-branches>`_. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know you just converted the text to a hint, which is an improvment. But reading this, it feels out of context. We're discussing making a new feature branch. Already having a PR from main means the user has gone two steps in a different direction. Also it's not really relevant to the feature branch.
I'm slightly in favor of removing this completely. Alternatively, if you think, this is still valuable (Do we expect users to find this when they need it? Is a PR from main really that bad that we need to discuss recovering from this non-standard workflow?), you can move this down to the section "Open a pull request".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is a PR from main really that bad that we need to discuss recovering from this non-standard workflow?
So this language has been in there for a while b/c from what I remember maintainers can't easily take over a PR made to main.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok let's get this in as an incremental improvement. We can discuss this separately.
Thanks @juanis2112! |
PR summary
(This change was done during SciPy2024 sprints)
It's a good practice to set up the branch tracking when pushing changes so I have changed the push command to have the tracking. However, this doesn't work in git versions lower than 1.7 so I added a tab without this for previous versions. This is how it looks like:
PR checklist