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

Add @util.deprecated decorator #1096

Merged
merged 4 commits into from
Oct 25, 2023
Merged

Add @util.deprecated decorator #1096

merged 4 commits into from
Oct 25, 2023

Conversation

richardm-stripe
Copy link
Contributor

@richardm-stripe richardm-stripe commented Oct 25, 2023

Background

The current approach for deprecating methods in stripe-python is "put a note in the docstring". See this conversation from last year (to the day).

This has some drawbacks

  • Deprecations do not emit warnings so they cannot be "captured" (e.g. by pytest).
  • Are not captured by type checkers, either.
  • No special IDE support. IDEs won't do the cute little "crossing out" of the deprecated method, or display a notice.

There is PEP 702, which isn't accepted yet, but is clearly en route to acceptance where Python will gain a standard @deprecated decorator, either in typing or in warnings. For now, a precursor is available typing_extensions.deprecated. This precursor

  • Emits a warning, so e.g. pytest will capture it by default.
  • Is specially detected by pyright, so VSCode with Pylance will cross it out, and running pyright in CI (with the reportDeprecated rule) will report. No Pycharm yet.

Unfortunately, @deprecated was introduced in typing_extensions 4.5.0 which isn't compatible with python 3.6.

Summary

This PR introduces @util.deprecated. If typing_extensions.deprecated is available, this is simply an alias to it. If not, it falls back to the implementation of typing_extensions.deprecated that I have pasted into the library.

At runtime, this will behave exactly the same as typing_extensions.deprecated, i.e. it will emit a DeprecationWarning warning. At type checking time it will specially recognized by Pyright and recieve special IDE support iff typing_extensions.deprecated is available.

This PR changes save to use @util.deprecated.
image

Alternatives

  1. We could continue with "mention in the docstring". I don't personally feel this is loud enough, you don't often review the docstrings of library code you aren't actively changing. Much of the purpose of deprecation is to prevent new usage but it's also important to provide advance notice to existing users that they should change their implementations.

  2. We could just warnings.warn(..., DeprecationWarning) inside the implementation of each deprecated function. This actually does get statically detected by Pycharm, but not by pyright. My view is that this isn't as future-proof as typing_extensions.deprecated. If PEP 702 is ratified, it's more likely that Pycharm, Mypy and the rest will add support for the PEP 702 than it is for IDEs beyond pycharm to start snooping inside implementations for warnings.warn(...)

  3. There's this PyPi package but as far as I can tell, it gives you no IDE deprecation detection, and would involve adding a dependency to stripe-python which I prefer to avoid if there's an alternative.

Aside

One concern last year was that warnings would pollute stderr at runtime. I am not concerned by this because the default warning filter is to squelch deprecation warnings outside of __main__. That is, if your entire app is one big main.py file and you call a deprecated stripe method in there, you will see a warning in stderr, but if (as I expect the vast majority do) you have broken up your app into modules and do not invoke stripe directly from the entry point, you will see no warning.

@richardm-stripe richardm-stripe requested review from a team and pakrym-stripe and removed request for a team October 25, 2023 15:46
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
Copy link
Contributor

@pakrym-stripe pakrym-stripe left a comment

Choose a reason for hiding this comment

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

Is py.typed change intentional?

@richardm-stripe richardm-stripe merged commit 455536e into master Oct 25, 2023
11 checks passed
@richardm-stripe richardm-stripe deleted the richardm-deprecation branch October 25, 2023 16:36
richardm-stripe added a commit that referenced this pull request Oct 25, 2023
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.

None yet

2 participants