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

Keyword only args for arguments like "drop" #5531

Closed
max-sixty opened this issue Jun 25, 2021 · 12 comments
Closed

Keyword only args for arguments like "drop" #5531

max-sixty opened this issue Jun 25, 2021 · 12 comments

Comments

@max-sixty
Copy link
Collaborator

Is your feature request related to a problem? Please describe.

A method like .reset_index has a signature .reset_index(dims_or_levels, drop=False).

This means that passing .reset_index("x", "y") is actually like passing .reset_index("x", True), which is silent and confusing.

Describe the solution you'd like
Move to kwarg-only arguments for these; like .reset_index(dims_or_levels, *, drop=False).

But we probably need a deprecation cycle, which will require some work.

Describe alternatives you've considered
Not have a deprecation cycle? I imagine it's fairly rare to not pass the kwarg.

@dcherian
Copy link
Contributor

👍 We should do something similar with all the decoding stuff in open_dataset and open_mfdataset (I think there's an issue for this).

@max-sixty
Copy link
Collaborator Author

Would we be happy to do without a deprecation cycle? I would vote lightly yes. (though partly because I think that would mean we did it sooner — if someone was up for doing the deprecation code then that switches it).

@gcaria
Copy link
Contributor

gcaria commented Jul 16, 2021

This is done neatly in pandas with a decorator (see here) that takes care of the deprecation.

Would it be fine to just implement the same in xarray?

@max-sixty
Copy link
Collaborator Author

For sure @gcaria ! Thanks for finding that

@dcherian dcherian changed the title kwarg-only args for arguments like "drop" Keyword only args for arguments like "drop" Mar 24, 2022
@dcherian
Copy link
Contributor

There's also a decorator by @hmaarrfk here.

Discussed in this comment: dask/dask#8934 (comment)

@mathause
Copy link
Collaborator

Both look good - but adding an intermittent dependency seems a bit heavy...

@hmaarrfk
Copy link
Contributor

I think in my readme i suggest vedoring the code.

Happy to give you a license for it so you don't need to credit me in addition to your own license.

@mathause
Copy link
Collaborator

I just discovered the scikit-learns decorator to deprecate positional args - see scikit-learn/scikit-learn#13311 - and now have a preference for this one. It seems the simplest and does not need any input as it reads the keyword-only args from the signature (could potentially add the version of the deprecation).

@_deprecate_positional_args
def f1(a, b, *, c=1, d=1):
    pass

and at the end of the deprecation period you only need to do

-@_deprecate_positional_args
def f1(a, b, *, c=1, d=1):
    pass

@mathause
Copy link
Collaborator

mathause commented Aug 11, 2022

I just realized that @hmaarrfk's decorator works similarly, but it also has some bells and whistles we probably don't need..?

@hmaarrfk
Copy link
Contributor

These decorators are kinda fun to write and are quite taylored to a certain release philosophy.

It might be warranted to just write your own ;)

@mathause
Copy link
Collaborator

I just opened #6910 - feedback welcome.

These decorators are kinda fun to write and are quite taylored to a certain release philosophy.
It might be warranted to just write your own ;)

I am glad that's fine for you.

@max-sixty
Copy link
Collaborator Author

Done!

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

No branches or pull requests

6 participants