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

decorator to deprecate positional arguments #6910

Merged
merged 3 commits into from
Aug 18, 2022

Conversation

mathause
Copy link
Collaborator

@mathause mathause commented Aug 12, 2022

Adds a helper function to deprecate positional arguments. IMHO this offers a good trade-off between magic and complexity. (As mentioned this was adapted from scikit-learn).

edit: I suggest to actually deprecate positional arguments in another PR.

@hmaarrfk
Copy link
Contributor

Are the functions you are considering using this functions that never had keyword arguments before? When I wrote a similar decorator before i had an explicit list of arguments that were allowed to be converted.

@headtr1ck
Copy link
Collaborator

Do you think that you can get it to work with static typing?

@mathause
Copy link
Collaborator Author

Are the functions you are considering using this functions that never had keyword arguments before? When I wrote a similar decorator before i had an explicit list of arguments that were allowed to be converted.

No, I want to consider arguments that have default values.

def mean(dim, skipna=None, keep_attrs=None):
    pass

@_deprecate_positional_args("v0.1.0")
def mean(dim, *, skipna=None, keep_attrs=None):
    pass

Or do I missunderstand your question?

Do you think that you can get it to work with static typing?

I think functools.wraps also attaches the annotations?

@mathause
Copy link
Collaborator Author

mathause commented Aug 12, 2022

I just realized that there is a bug in the decorator for "keyword-only arguments without a default". I wonder if I should fix this or just disallow this pattern, i.e. the following does currently not work properly:

@_deprecate_positional_args("v0.1.0")
def func(a, *, b):
    pass

but I am not sure how helpful it is.

EDIT: It now raises a TypeError for this case.

@mathause
Copy link
Collaborator Author

I also just realized, that we made many arguments keyword-only without deprecation (e.g. xr.Dataset.mean) - but that should not stop us from using this - there are still a lot of instances where it could be helpful.

@mathause
Copy link
Collaborator Author

Any objections to merge this as is?

@dcherian
Copy link
Contributor

👍 put it on the meeting agenda for today

Comment on lines +62 to +63
print(f"{args=}")
print(f"{pos_or_kw_args=}")
Copy link
Member

Choose a reason for hiding this comment

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

are these print statements intentional?

Copy link
Contributor

Choose a reason for hiding this comment

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

@mathause I think you missed these print statements?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! Will fix them in a follow up!

@@ -0,0 +1,86 @@
import inspect
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest keeping the original copyright notice at the top of the this file.

@mathause mathause merged commit 98deab5 into pydata:main Aug 18, 2022
@mathause mathause deleted the deprecate_positional_helper branch August 18, 2022 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants