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.

Sign up for GitHub

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

Jump to bottom

DEP: Use delimiter rather than delimitor as kwarg in mrecords #19921

Merged
merged 2 commits into from Sep 28, 2021
Merged

DEP: Use delimiter rather than delimitor as kwarg in mrecords #19921

merged 2 commits into from Sep 28, 2021

Conversation

DimitriPapadopoulos
Copy link
Contributor

@DimitriPapadopoulos DimitriPapadopoulos commented Sep 21, 2021 •
edited

First revert the previous commit, too optimistic (#19911).

Then propose a new way to fix the typo, without breaking backwards compatibility.

@DimitriPapadopoulos DimitriPapadopoulos mentioned this pull request Sep 21, 2021
Merged
@DimitriPapadopoulos
Copy link
Contributor Author

Should I add a deprecation warning, in the style of numpy.deprecate()?

@seberg
Copy link
Member

seberg commented Sep 22, 2021

I suppose considering how exotic this function is, we can likely get away with changing it. But it will have to be a proper deprecation then: give a deprecation warning (with a test), so that we can officially remove it in two versions.

in the style of numpy.deprecate()?

You can probably ignore that... Just give a Deprecation warning, and end on a note about the current version of NumPy (1.22). (You can search the code for DEPRECATED or DeprecationWarning, for examples I guess).

@DimitriPapadopoulos
Copy link
Contributor Author

After searching the source code, it appears I have to read NEP 23 — Backwards compatibility and deprecation policy. Thanks for the hints.

@DimitriPapadopoulos
Copy link
Contributor Author

Clearly this function is not being used much...
https://searchcode.com/?q=numpy%20fromtextfile&lan=19
https://sourcegraph.com/search?q=context:global+fromtextfile+lang:Python&patternType=literal

@DimitriPapadopoulos DimitriPapadopoulos force-pushed the codespell-rev-delimitor branch 5 times, most recently from ae020cf to 71819cb Compare September 22, 2021 13:15
seberg
seberg reviewed Sep 22, 2021
View reviewed changes
numpy/ma/mrecords.pyi Outdated Show resolved Hide resolved
numpy/ma/tests/test_deprecations.py Outdated Show resolved Hide resolved
numpy/ma/tests/test_deprecations.py Outdated Show resolved Hide resolved
numpy/ma/mrecords.py Outdated Show resolved Hide resolved
numpy/ma/tests/test_deprecations.py Outdated Show resolved Hide resolved
@DimitriPapadopoulos DimitriPapadopoulos force-pushed the codespell-rev-delimitor branch 2 times, most recently from bac1c61 to ac1cc32 Compare September 22, 2021 14:43
eric-wieser
eric-wieser reviewed Sep 22, 2021
View reviewed changes
numpy/ma/mrecords.py Outdated Show resolved Hide resolved
@DimitriPapadopoulos DimitriPapadopoulos force-pushed the codespell-rev-delimitor branch 4 times, most recently from 506ce9d to 394d243 Compare September 22, 2021 17:19
@seberg seberg changed the title MAINT: Fix typo in public API DEP: Use delimiter rather than delimitor as kwarg in mrecords Sep 22, 2021
bashtage
bashtage suggested changes Sep 23, 2021
View reviewed changes
Copy link
Contributor

@bashtage bashtage left a comment

Choose a reason for hiding this comment

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

I think the warning could be clarified in case someone uses positional arguments.

numpy/ma/mrecords.py Outdated Show resolved Hide resolved
@DimitriPapadopoulos DimitriPapadopoulos force-pushed the codespell-rev-delimitor branch 5 times, most recently from 80b181e to e7211af Compare September 23, 2021 15:47
seberg
seberg approved these changes Sep 23, 2021
View reviewed changes
Copy link
Member

@seberg seberg left a comment

Choose a reason for hiding this comment

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

I am happy with this changes right now. Unless anyone has more comments or thinks we just should do this (or feels this is not fringe enough to skip announcing to the mailing list, just comment).

It probably should get a very brief release note for completeness sake though, those are files in doc/release/upcoming_changes with the PR number in the name (there is a README).

One last note: It is possible to pass the argument by position as a work-around for anyone who must support multiple versions.

@seberg seberg added the 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes label Sep 23, 2021
@DimitriPapadopoulos DimitriPapadopoulos force-pushed the codespell-rev-delimitor branch 2 times, most recently from 4f354d5 to 80d3b7b Compare September 23, 2021 16:21
@DimitriPapadopoulos
Copy link
Contributor Author

I have added doc/release/upcoming_changes/19921.deprecation.rst.

@DimitriPapadopoulos DimitriPapadopoulos force-pushed the codespell-rev-delimitor branch 2 times, most recently from bab8db0 to a963463 Compare September 26, 2021 18:14
@DimitriPapadopoulos
REV: 8396026
e874e13 
Revert delimitor -> delimiter as the parameter name is part of the
public API. We'll find a different solution to fix that.
@DimitriPapadopoulos @eric-wieser
MAINT: Fix typo in public API
cbfa658 
By keeping both variants of the keyowrd parameter, we achieve backwards
compatibility. The old mispelled variant has been removed from the
documentation, the new variant does appear in the documentation and
using noth variants raises a TypeError.

Additionally, the old variant can only be used as a keyword argument,
not as a positional argument.

Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
@DimitriPapadopoulos
Copy link
Contributor Author

I just noticed this corner case, where a deprecation warning is emitted but delimitor overwrites delimiter:

fromtextfile(fname, delimiter=None, delimitor=";")

A bit contrived, but I thought I'd mention it anyway.

I can't understand how the CI failure is related to this PR. I have rebased and re-pushed, hopefully the failure will go away by itself.

@seberg
Copy link
Member

seberg commented Sep 28, 2021

Yeah, I was aware of that possibility, but passing both is so weird, that I don't think it matters. Lets give this a shot. Thanks @DimitriPapadopoulos.

@seberg seberg merged commit 7d2904b into numpy:main Sep 28, 2021
@DimitriPapadopoulos DimitriPapadopoulos deleted the codespell-rev-delimitor branch September 28, 2021 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eric-wieser eric-wieser eric-wieser left review comments

@bashtage bashtage bashtage requested changes

@BvB93 BvB93 BvB93 left review comments

@seberg seberg seberg approved these changes

Assignees
No one assigned
Labels
03 - Maintenance 07 - Deprecation 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@DimitriPapadopoulos @seberg @eric-wieser @bashtage @BvB93