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

refactor(python): Improve deprecation utils #10167

Merged
merged 3 commits into from
Jul 31, 2023
Merged

Conversation

stinodego
Copy link
Member

@stinodego stinodego commented Jul 30, 2023

Partially addresses #8004

Followup of #10147

Changes:

  • Refactor the deprecated_alias util
    • Renamed to deprecate_argument_name deprecate_renamed_parameter
    • Only renames a single argument. Stack multiple decorators to rename multiple arguments.
    • Requires a version keyword argument to specify when the deprecation happened.
  • Rename the redirect util to deprecate_renamed_methods. Now requires a versions keyword argument.
  • Rename some other utils and improve docstrings.

I think this now gives us quite a solid set of utils for doing deprecations:

  • issue_deprecation_warning
  • deprecate_function
  • deprecate_renamed_function
  • deprecate_renamed_parameter
  • deprecate_renamed_methods
  • deprecate_nonkeyword_arguments

@alexander-beedie I would appreciate your input on these changes, as I know you've originally written most of these utils!

EDIT: Actually, probably the deprecate_renamed_methods decorator should also be a 'stacking' decorator redirecting only 1 method. Putting this on 'draft' for now until I have time to update. I can't figure this one out - I don't think it can be done in the current setup as a class decorator overwriting the __getattr__ dunder. Let's leave it like this until we come up with a better approach.

@github-actions github-actions bot added internal An internal refactor or improvement python Related to Python Polars labels Jul 30, 2023
@stinodego stinodego marked this pull request as ready for review July 30, 2023 00:53
@stinodego stinodego marked this pull request as draft July 30, 2023 06:10
@alexander-beedie
Copy link
Collaborator

alexander-beedie commented Jul 31, 2023

I like the inclusion of versioning info (and some of the renames are definite wins).

So, a few small suggestions!

Instead of...

@deprecate_argument_name("fmt", "format", version="0.17.3")

...how about this:

@deprecate_param_name( before_after={"fmt": "format"}, as_of="0.17.3")

You could still stack the decorator if the same method experiences multiple separate deprecations (let's hope this is rare, heh), but if more than one param is changed at a time then the dict format connects those changes better, and before_after reads well. (I also prefer "param" -or "parameter"- to "argument", but that's not a tightly-held opinion ;)

Using as_of instead of just version might also be preferable; version by itself doesn't contain the implication that this is when the deprecation was initiated, whereas as_of does. In conjunction with before_after it almost reads as a sentence, with clear intent.

@stinodego
Copy link
Member Author

I also prefer "param" -or "parameter"- to "argument"

I think "parameter" is more correct here. Changed to deprecate_renamed_parameter (in line with the other deprecate_renamed_x utils).

You could still stack the decorator if the same method experiences multiple separate deprecations (let's hope this is rare, heh)

I don't really like the dict input here. As you mention, it's a rare occurrence that we would deprecate multiple args in the same version, and in the 98% of cases where this is NOT the case, the flat parameters old/new make more sense. If we do have multiple deprecations, they are adequately coupled by having the same version number.

version by itself doesn't contain the implication that this is when the deprecation was initiated, whereas as_of does

Agreed, though as_of does not convey that a version number should be passed- someone might specify a date or something. as_of_version would be most complete, but is a bit long. Why is naming things so hard 😅 I think version is clear enough in the context of a deprecation util. It's the same language as the deprecated library which I think is has a nice enough API (though too simplistic for our purposes).

@stinodego stinodego marked this pull request as ready for review July 31, 2023 11:22
@alexander-beedie
Copy link
Collaborator

alexander-beedie commented Jul 31, 2023

Why is naming things so hard 😅

When this is our hardest problem we're doing pretty well! 😁

I don't really like the dict input here. As you mention, it's a rare occurrence that we would deprecate multiple args in the same version

Yup, don't disagree; no need to prioritise the edgiest of edge-cases.

Agreed, though as_of does not convey that a version number should be passed- someone might specify a date or something.

True... however, my first thought on seeing version was that I needed to check if it was the target version for the deprecation period to be over (eg: not deprecated "as of" the given version, but deprecated "until" the given version - at which point it would stop working). If we were to keep "as_of" it would be trivial/cheap to check that it is passed a string that looks like a version number? 🤔 However, if this is a standard param elsewhere then it's probably fine.

@stinodego
Copy link
Member Author

stinodego commented Jul 31, 2023

All right, I'll go ahead with the current version. We can update later if we think of the perfect variable name :) Thanks for the input!

@stinodego stinodego merged commit f00b47d into main Jul 31, 2023
12 checks passed
@stinodego stinodego deleted the more-deprecation-update branch July 31, 2023 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal An internal refactor or improvement python Related to Python Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants