Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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 uson gitter for real-time discussion.
For details on testing, writing docs, and our review process, please seethe developer guide
We strive to be a welcoming and open project. Please follow ourCode of Conduct.
timhoffm commentedJul 14, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 thathttps://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 thesprints 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 |
juanis2112 commentedJul 14, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
🚀 I would find a diagram of the workflow helpful. There are many visualization variants out there. I liktethis 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. |
QuLogic commentedJul 18, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
54df556
to315fbb3
CompareThanks@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 |
QuLogic commentedJul 20, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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
tob488524
CompareDelete tab for branch tracking workflowApply suggestions from code reviewCo-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
b488524
to9961c6f
Comparedoc/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 oftypying your branch name every time, you can once do:: | |
Instead oftyping 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"
Uh oh!
There was an error while loading.Please reload this page.
``--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.
30f803b
intomatplotlib:mainUh oh!
There was an error while loading.Please reload this page.
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