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

API: Remove deprecated msort function #24494

Merged
merged 2 commits into from Aug 22, 2023

Conversation

mtsokol
Copy link
Member

@mtsokol mtsokol commented Aug 22, 2023

Hi @rgommers @ngoldbaum,

msort was deprecated in 1.24 - this small PR removes it completely. (Plus some leftovers from removed geterrobj).

@mtsokol mtsokol changed the title API: Remove deprecated msort function API: Remove deprecated msort function Aug 22, 2023
@rgommers rgommers added this to the 2.0.0 release milestone Aug 22, 2023
Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @mtsokol. Could you add a release note in doc/release/upcoming_changes/24494.expired.rst?

@mtsokol
Copy link
Member Author

mtsokol commented Aug 22, 2023

LGTM, thanks @mtsokol. Could you add a release note in doc/release/upcoming_changes/24494.expired.rst?

Added! And how about release notes for all the previous work that was merged? Should I provide a separate PR?

@rgommers
Copy link
Member

Added! And how about release notes for all the previous work that was merged? Should I provide a separate PR?

Hmm yeah, it's a little tricky. The current release note system is not ideal for listing many changes, and it won't assemble it well into a readable list. So I am not sure what's best. Could use towncrier snippets and then clean them up into a sane table at the end, or just keep a single list in a .rst file or an issue for now. @charris do you have any preference here?

@charris
Copy link
Member

charris commented Aug 22, 2023

do you have any preference here?

I'd need to see the notes to judge. It is possible to put lists in snippets, the only PR specific bit is the link to the PR. The trick is to keep track of where the documentation is and remembering to use it in the notes. I suppose one could also make an empty PR and use it to list all the changes. For that matter, one could also edit the note itself.

@rgommers
Copy link
Member

I suppose one could also make an empty PR and use it to list all the changes. For that matter, one could also edit the note itself.

Either one of these seems better indeed. Having a list in one place will be useful, rather than scattered over a ton of snippets. Maybe editing doc/source/release/2.0.0-notes.rst in one new PR now with the changes to date, after running towncrier once to assemble what's already there?

@rgommers rgommers merged commit 3032e84 into numpy:main Aug 22, 2023
52 checks passed
@rgommers
Copy link
Member

This one is really straightforward, so in it goes.

@mtsokol mtsokol deleted the remove-msort-function branch August 22, 2023 20:07
@charris
Copy link
Member

charris commented Aug 22, 2023

after running towncrier once to assemble what's already there?

I think that is reasonable. I think towncrier deletes the used snippets by default. It also used to make a separate note, but IIRC, it now adds to the existing note. In any case as long as the material is there, we can make it work at release time.

towncrier build --version "<version>"

Should work. Note that working in a branch should make this safe to experiment with.

@rgommers
Copy link
Member

Sounds good. @mtsokol would you be able to do this? A separate PR with two commits - one for processing the existing snippets, and one for your list of additions/removals - would be useful.

@mtsokol
Copy link
Member Author

mtsokol commented Aug 23, 2023

Sounds good. @mtsokol would you be able to do this? A separate PR with two commits - one for processing the existing snippets, and one for your list of additions/removals - would be useful.

Sure! I will create one PR for changes that were already merged (for Part 3 and Part 4 I will add changes to PRs directly)

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

Successfully merging this pull request may close these issues.

None yet

4 participants