-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
MNT: Replace master with main #11379
Conversation
This might have to wait for other repos in the org to catch up. Also, I think we need to backport to avoid broken links... or we need to keep |
Hello @pllim 👋! It looks like you've made some changes in your pull request, so I've checked the code again for style. There are no PEP8 style issues with this pull request - thanks! 🎉 Comment last updated at 2021-04-02 22:27:27 UTC |
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.
In some places, the master->main changes don't make much sense, but overall these changes only depend on the APE and ci-helpers repos, so we're in a pretty good shape.
docs/io/ascii/fast_ascii_io.rst
Outdated
@@ -144,7 +144,7 @@ relatively unimportant, this converter might be the best option in | |||
performance-critical scenarios. | |||
|
|||
`Here | |||
<https://nbviewer.jupyter.org/github/astropy/astropy-notebooks/blob/master/io/ascii/conversion_profile.ipynb>`__ | |||
<https://nbviewer.jupyter.org/github/astropy/astropy-notebooks/blob/main/io/ascii/conversion_profile.ipynb>`__ |
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.
Unrelated to this PR, but are these notebooks are not tested and in fact don't work any more, so better to remove them from the docs altogether.
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 continue discussion at #11381.
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.
This will also require re-naming that repo's branch, but I'll just do that right now since it's incredibly low traffic.
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.
Notebooks are removed. But if we want to keep that repo, should just rename the branch there anyway though that is out of scope of this PR now.
I'll try to give this a proper review soon, but one immediate thought: I think this arguably belongs in 4.2.x not 4.0.x - the LTS in principle shouldn't have anything to do with FWIW, I don't necessarily object to this, but if the backport is going to take significantly more effort for LTS I think it's not cirtical. Regardless, I don't think it needs to block 4.0.5 so I'm going to re-milestone it to the next LTS. |
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.
LGTM!
But one procedural question: I should probably merge this at the same time as making the name change, right, so that there's a minimal time of being out-of-sync? If so were you thinking I should make the README update as a push to master, or did you want to add this here, @pllim?
@@ -5,7 +5,7 @@ Unified File Read/Write Interface | |||
|
|||
``astropy`` provides a unified interface for reading and writing data in | |||
different formats. For many common cases this will streamline the process of | |||
file I/O and reduce the need to master the separate details of all of the I/O | |||
file I/O and reduce the need to learn the separate details of all of the I/O |
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 know, I'm not really sure this change is necessary since I think this is the one use of "master" that has a different etymology. But "learn" actually works fine here and sounds better to me anyway. Just not something we necessarily have to stay vigilant about in the future 🤷.
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.
It was a blind sed
run and I thought "might as well"...
docs/io/ascii/fast_ascii_io.rst
Outdated
@@ -144,7 +144,7 @@ relatively unimportant, this converter might be the best option in | |||
performance-critical scenarios. | |||
|
|||
`Here | |||
<https://nbviewer.jupyter.org/github/astropy/astropy-notebooks/blob/master/io/ascii/conversion_profile.ipynb>`__ | |||
<https://nbviewer.jupyter.org/github/astropy/astropy-notebooks/blob/main/io/ascii/conversion_profile.ipynb>`__ |
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.
This will also require re-naming that repo's branch, but I'll just do that right now since it's incredibly low traffic.
Ideally, this should be merged soon after you change the default branch. Given CI was failing, might want to retrigger to make sure I didn't break anything after name change but before merge.
I already updated the README as part of this PR. Or are you referring to something else? |
Oh, sorry, @pllim, I meant specifically the README update along the lines of here or similar: astropy/astropy-project#151 (comment) - you did indeed fix the to-be-broken links, I just meant the informational addition. |
Oh. Feel free to push to this PR or as follow-up PR. The discussions were quiet long and I forgot what the conclusion was. 😅 |
6639c33
to
ad2e05e
Compare
@eteq , this should be ready, except for the remaining README change that you mentioned in #11379 (comment) , which can be handled separately. |
@pllim - the changes related to the io.ascii notebooks look fine, thanks! |
Revert out-of-scope changes. Replace some words in narrative. Apply suggestions from bsipocz DOC: Remove mentioned of astropy-notebooks Co-authored-by: Brigitta Sipőcz <b.sipocz@gmail.com>
Update: Originally, I had a change log entry ( |
changelog updated in e174a67 following the new towncrier method |
I'm doing it! |
MNT: Replace master with main
This PR attempts to replace all mention of
master
tomain
in this repository. This should be reviewed and merged right after the maintainer has renamed the default branch.This is an automated update made by the
batchpr
tool 🤖 - feel free to close if it doesn't look good! You can report issues to @pllim.This is part of #11378
Fix #11381
TODO