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

gh-104003: Implement PEP 702 #104004

Merged
merged 25 commits into from Nov 29, 2023
Merged

gh-104003: Implement PEP 702 #104004

merged 25 commits into from Nov 29, 2023

Conversation

JelleZijlstra
Copy link
Member

@JelleZijlstra JelleZijlstra commented Apr 30, 2023

@JelleZijlstra
Copy link
Member Author

This is copied from python/typing_extensions#105.

Lib/test/test_typing.py Outdated Show resolved Hide resolved
Lib/test/test_typing.py Outdated Show resolved Hide resolved
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Waiting for the other reviewers to pipe in? This seems pretty straightforward.

@AlexWaygood
Copy link
Member

Waiting for the other reviewers to pipe in? This seems pretty straightforward.

The PEP hasn't been accepted yet!

@JelleZijlstra
Copy link
Member Author

Planning to hit the merge button the moment the PEP is accepted :)

@JelleZijlstra
Copy link
Member Author

PEP 702 has been accepted, but it's changed since this PR. I'll update the PR soon to put the decorator in warnings and sync the implementation from typing-extensions.

Doc/glossary.rst Outdated Show resolved Hide resolved
Doc/library/warnings.rst Outdated Show resolved Hide resolved
Doc/library/warnings.rst Outdated Show resolved Hide resolved
Doc/library/warnings.rst Outdated Show resolved Hide resolved
Doc/library/warnings.rst Outdated Show resolved Hide resolved
Doc/whatsnew/3.13.rst Outdated Show resolved Hide resolved
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
@AlexWaygood
Copy link
Member

Would you be open to implementing this as a class, to support downstream introspection, rather than an inner function?

Hmm, the current implementation sets the __deprecated__ dunder attribute on the object returned from deprecated(). The purpose of that is supposed to be to allow third-party runtime tools to detect that the original object was decorated with @deprecated. Is that insufficient?

it'd be nicer if we could just use isinstance() 🙂

If you want to use isinstance(), you could create a runtime-checkable protocol for that ;)

from typing import Protocol, runtime_checkable

@runtime_checkable
class DeprecatedProto(Protocol):
    __deprecated__: str

@AlexWaygood AlexWaygood self-requested a review November 26, 2023 18:55
@Viicos
Copy link
Contributor

Viicos commented Nov 26, 2023

The purpose of that is supposed to be to allow third-party runtime tools to detect that the original object was decorated with @deprecated. Is that insufficient?

The issue is when using deprecated the way Zac-HD wrote (Annotated[T, warnings.deprecated("message")]), no object is being decorated here (in other words, deprecated("message") returns another function yet to be called with the decorated object)

@AlexWaygood
Copy link
Member

AlexWaygood commented Nov 26, 2023

The purpose of that is supposed to be to allow third-party runtime tools to detect that the original object was decorated with @deprecated. Is that insufficient?

The issue is when using deprecated the way Zac-HD wrote (Annotated[T, warnings.deprecated("message")]), no object is being decorated here (in other words, deprecated("message") returns another function yet to be called with the decorated object)

Got it. What if we did something like this?

diff --git a/Lib/warnings.py b/Lib/warnings.py
index 36da0e75c6..fc8a10e71f 100644
--- a/Lib/warnings.py
+++ b/Lib/warnings.py
@@ -621,6 +621,7 @@ def wrapper(*args, **kwargs):
                 f"a class or callable, not {arg!r}"
             )

+    decorator.__origin__ = deprecated
     return decorator

Then third-party code could do checks like this:

from typing import get_origin
from warnings import decorated

if get_origin(obj) is deprecated:
    ...  # it was a wrapper function returned by `warnings.deprecated("message")`

@Viicos
Copy link
Contributor

Viicos commented Nov 26, 2023

Doesn't seem to be get_origin's primary use (maybe docs should be updated), but that would avoid completely refactoring the current implementation!

@AlexWaygood
Copy link
Member

AlexWaygood commented Nov 26, 2023

Doesn't seem to be get_origin's primary use

It's already sorta awkwardly overloaded in what it means tbh — the __origin__ attribute of ParamSpecArgs instances doesn't really have the same semantics as the __origin__ attribute of generic aliases :)

@Zac-HD
Copy link
Contributor

Zac-HD commented Nov 26, 2023

A class seems somewhat more elegant to me, but yeah, that would work.

@JelleZijlstra
Copy link
Member Author

I'm OK with making it a class. It's not what the object is meant for, but it's a reasonable extension.

Using __origin__ feels hacky and wouldn't provide an easy way to get the deprecation message out.

Lib/warnings.py Outdated Show resolved Hide resolved
Lib/warnings.py Show resolved Hide resolved
Comment on lines 338 to 340
* The new :func:`warnings.deprecated` decorator provides a way to communicate
deprecations to :term:`static type checkers <static type checker>` and
to warn on usage of deprecated classes and functions.
Copy link
Member

Choose a reason for hiding this comment

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

Despite static typing being the original motivation for the feature, I would actually put the runtime effect first here:

Suggested change
* The new :func:`warnings.deprecated` decorator provides a way to communicate
deprecations to :term:`static type checkers <static type checker>` and
to warn on usage of deprecated classes and functions.
* The new :func:`warnings.deprecated` decorator provides an ergonomic way to
mark a class or function as deprecated. A deprecation warning will be
emitted whenever a decorated function or class is used at runtime. The
decorator is also understood by
:term:`static type checkers <static type checker>`, which will emit warnings
if they identify a decorated function or class being used.

Copy link
Member Author

Choose a reason for hiding this comment

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

I still want to put the type checker effect first because that's the unique part. You can write a decorator that generates runtime warnings yourself; the new and exciting part is that this decorator is also understood by static type checkers.

Copy link
Member

Choose a reason for hiding this comment

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

You can do, sure, but do people? It was possible to reimplement itertools.batched in a few lines of code before Python 3.12, but lots of people were still very excited by its inclusion in the stdlib in Python 3.12.

I think this new decorator here could prove pretty popular with people who don't use static typing! But, I don't feel strongly; it looks fine to me now :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, there are lots of third-party deprecation decorators. The Deprecated library, I think Flask has one internally (David Lord mentioned it in PEP 702 discussions), I know at Quora we have one internally.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if that's meant to be a point in agreement or in disagreement with the point I'm making that this decorator could prove pretty popular with people who don't use static typing. Anyway, as I say, I'm happy with the docs now!

Doc/library/warnings.rst Outdated Show resolved Hide resolved
Doc/library/warnings.rst Outdated Show resolved Hide resolved
@JelleZijlstra
Copy link
Member Author

Thanks @AlexWaygood for the review! I pushed some changes.

Doc/library/warnings.rst Show resolved Hide resolved
Lib/test/test_warnings/__init__.py Outdated Show resolved Hide resolved
Lib/test/test_warnings/__init__.py Outdated Show resolved Hide resolved
Lib/warnings.py Outdated Show resolved Hide resolved
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

LGTM

Lib/warnings.py Outdated Show resolved Hide resolved
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@JelleZijlstra JelleZijlstra merged commit d4a6229 into python:main Nov 29, 2023
29 checks passed
@JelleZijlstra JelleZijlstra deleted the pep702 branch November 29, 2023 17:38
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
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

8 participants