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

Improve handling of deprecations in stubs #870

Open
Daverball opened this issue Feb 14, 2024 · 3 comments
Open

Improve handling of deprecations in stubs #870

Daverball opened this issue Feb 14, 2024 · 3 comments

Comments

@Daverball
Copy link
Contributor

Daverball commented Feb 14, 2024

This is mostly a meta issue, I've noticed that when upgrading the stubs for version 2.1.0 the deprecated methods/overloads have been immediately removed.

We now have typing_extensions.deprecated, which I think should be preferred in such cases, since it will create vastly more readable error messages, compared to "No overload variant matches argument types" in the case of deprecating index=1 for DataFrame.groupby or "Series[Any] not callable" in the case of renaming DataFrame.applymap.

In the former case we could've added a deprecated overload that still accepts ColumnAxis and in the latter made a copy of the method and applied the decorator to the deprecated method name.

@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Feb 14, 2024

This is mostly a meta issue, I've noticed that when upgrading the stubs for version 2.1.0 the deprecated methods/overloads have been immediately removed.

We now have typing_extensions.deprecated, which I think should be preferred in such cases, since it will create vastly more readable error messages, compared to "No overload variant matches argument types" in the case of deprecating index=1 for DataFrame.groupby or "Series[Any] not callable" in the case of renaming DataFrame.applymap.

In the former case we could've added a deprecated overload that still accepts ColumnAxis and in the latter made a copy of the method and applied the decorator to the deprecated method name.

I wasn't aware of the new deprecated operator that would work in stub files. Seems that was only accepted a few months ago!

Prior to that, we've had a policy of removing deprecated behavior in the stubs to encourage people to stop using deprecated behavior. Going forward, we can see if we can make @deprecated work within our testing scheme.

@twoertwein Your thoughts?

@twoertwein
Copy link
Member

I'm fine accepting PRs that use it! I'm not sure whether that will create more maintenance burden on our side:

  • we might need to create/split overloads to pin-point the deprecated behavior
  • tests for deprecated behaviors
  • have more ignore comments (e.g., if we cannot immediately use keyword-only arguments)

xref #848 (comment)

I think we should definitely use this decorator for breaking changes in major releases (that currently do not warn; pandas 3.0). Currently we silence these cases in the CI but users do not get a heads up.

@Daverball
Copy link
Contributor Author

If you're worried about maintenance burden, a reasonable compromise could be to only use deprecated in simple cases, such as renaming/removing a method/function/class. In these cases you would have to add stubtest ignore entry anyways, that you later have to remove again, so in terms of effort they seem about the same to me. At least this way you don't have to edit two separate files and stubtest will tell you when it is time to remove the deprecated entity.

I.e. in the case of renaming applymap to map you would use it, but for the other case, where you need to add an additional overload you wouldn't, unless someone already went through the trouble of writing tests for it.

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

No branches or pull requests

3 participants