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 Deprecated #34

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

adriangb
Copy link
Contributor

cc @tiangolo @JelleZijlstra @samuelcolvin

Open question: can we design this in a way to be compatible with the version of @typing.deprecated that returns something useful at runtime? It would be nice if it returns a class that we can insert into the bases of this one so that anyone doing something like for annotation in annotations: ... isinstance(annotation, typing.Deprecated is okay.

@codecov-commenter
Copy link

Codecov Report

Merging #34 (b5e6ccb) into main (03b79a5) will decrease coverage by 3.62%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff             @@
##              main      #34      +/-   ##
===========================================
- Coverage   100.00%   96.38%   -3.62%     
===========================================
  Files            1        2       +1     
  Lines           59      166     +107     
  Branches         5       47      +42     
===========================================
+ Hits            59      160     +101     
- Misses           0        6       +6     
Impacted Files Coverage Δ
annotated_types/__init__.py 94.39% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

otherwise LGTM.

class Deprecate(BaseMetadata):
"""Marks a parameter or field as deprecated"""

message: str
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe add the other parameters? But I don't mind that much.

@JelleZijlstra
Copy link
Contributor

@adriangb There is no active proposal for typing.Deprecated, only for the decorator. However, a compatible approach probably would be to set a .__deprecated__ attribute.

@adriangb
Copy link
Contributor Author

Right I made that up as something that could plausibly be returned by @deprecated.

Would you make __deprecated__ a boolean, or try to include the deprecation reason and such?

@samuelcolvin
Copy link
Contributor

I'd be keen to move ahead with this - as part of solving pydantic/pydantic#2255.

@Zac-HD WDYT?

@tiangolo
Copy link
Contributor

So cool! 🚀

My only thought is that I would like to work on my thingy doc() to make it compatible with deprecated().

I imagine there could be code/parameters that used both (I will in FastAPI and others), e.g.:

def say_hi(
    *,
    name: Annotated[
        str | None,
        deprecated("Please use first_name"),
        doc("The full name of the user"),
    ] = None,
    first_name: Annotated[
        str | None,
        doc("The first name of the user")
    ] = None,
):
    return "Hello"

In this case, the new alias/generic Deprecated[str] would not make it easy/obvious to use both (or more annotations).

@Zac-HD
Copy link
Member

Zac-HD commented May 13, 2023

I'm keen for annotated-types to support some way of expressing a deprecation, and matching the runtime and static-check-time semantics of an accepted PEP-702 would be a great way to do that. Implications:

  • We should probably hold off on shipping this until the PEP is accepted, or else mark everything downstream as alpha/conditional-on-acceptance and be ready to revert if it's rejected or substantially ammended.
    • I prefer the former, because (a) I remember the churn when PEP-678 notes went from one string to a tuple of strings, and (b) I want to avoid even appearing to pressure the steering council into a particular decision.
  • We should specify that this metadata MUST NOT be used on a required argument: you must either (a) have a default value, (b) have another overload where it's not required, or (c) use @typing.deprecated on the function or method instead.
    • annotated-types can't enforce that but I think Pydantic could? I guess you might set the .__deprecated__ attribute instead of raising an error 🤔

I'd also prefer to require Annotated[T, Deprecated["message"]] rather than Deprecated[T] - the latter means we have two things, and since @tiangolo's deprecated() would return the as-yet-undocumented Deprecate type we'd need to find sensible and intuitive names for both of them.

From reading the PEP I get the sense that @JelleZijlstra doesn't want to propose a new typing.Deprecated[T, "message"] type because (among other reasons?) it's the first time in the static type system that "message" is just a string, not a forward reference to a type. Technically solvable by requiring Deprecated[T, Literal["..."]] but that's not my favorite DX ever.

@tiangolo
Copy link
Contributor

Great!


Just to clarify a couple of things.

deprecated() as a decorator is what is already on PEP 702 by @JelleZijlstra , the usage as Deprecated[x] is in the rejected ideas (at least for now, at least for this PEP).

But having deprecated() return an inspectable object that is not only used as a decorator is not forbidden (or defined) in the PEP. (Taken from my email conversations with Jelle).

The thing I've been working on is a doc() function, that would be used only inside of Annotated and would have docs for parameters, class attributes (or TypedDicts), etc. My original idea when I started working on that was to include more information, deprecated among other things, and to allow using it in Annotated or as a decorator, but all that is now discarded in favor of Jelle's deprecated() potential usage inside of Annotated. My working doc is here: https://github.com/tiangolo/fastapi/blob/typing-doc/typing_doc.md, but I haven't updated it to remove the extra stuff (deprecated and anything else apart from the parameter "doc string").

I think Jelle and I started working on both things at similar times and I didn't know about deprecated(), then we had a couple of emails and the conclusion was that it's better for me to drop everything else apart from the doc string for parameters in doc(), and that supporting (or not) using deprecated() inside of Annotated could be done afterwards on top of the PEP. And that way, both ideas would be compatible. Also, as deprecated() would take care of deprecating and doc() would not have that, doc() would no longer be a decorator, only for Annotated (as anywhere it could be used as a decorator a plain docstring would do).


In short, the closer to "standard" would actually (probably) be

Annotated[T, deprecated("message")]

rather than

Annotated[T, Deprecated["message"]] or Deprecated[T].

Just because deprecated() (and not Deprecated[]) is what's already in the process of becoming a PEP. So, a function, not a generic.

And it would be compatible with my future wannabe-PEP-thing as in Annotated[T, deprecated("message"), doc("some param")].

@adriangb
Copy link
Contributor Author

And it would be compatible with my future wannabe-PEP-thing as in Annotated[T, deprecated("message"), doc("some param")].

Can't you then do something like Deprecated = Annotated[T, deprecated("This parameter/field has been deprecated")] so that effectively FastAPI users can do x: Deprecated[int] if they don't care about setting a specific deprecation message?

As far as this PR is concerned I agree with Zac that we might want to wait until (1) PEP 702 is accepted and (2) typing.deprecated returns a runtime-inspectable object that we can just tell users to use here (and maybe provide the shorthand Annotated type alias).

@JelleZijlstra
Copy link
Contributor

From reading the PEP I get the sense that @JelleZijlstra doesn't want to propose a new typing.Deprecated[T, "message"] type because (among other reasons?) it's the first time in the static type system that "message" is just a string, not a forward reference to a type. Technically solvable by requiring Deprecated[T, Literal["..."]] but that's not my favorite DX ever.

For me the main reason is that the vast majority of deprecations in the stdlib could be marked without typing.Deprecated, and I didn't want to make the PEP more complicated than necessary. Going through all the discussions already took a lot of energy. If PEP 702 does get accepted, I'd be happy to sponsor a follow-up PEP to provide a richer deprecation API.

I'm personally not too concerned about the strings point, but some type checker authors find this important.

message: str


Deprecated = Annotated[T, Deprecate(message='')]
Copy link
Contributor

@Viicos Viicos Nov 26, 2023

Choose a reason for hiding this comment

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

Isn't having both Deprecate and Deprecated confusing? Someone may import the wrong one and do my_field: Annotated[str, Deprecated('')] instead of my_field: Annotated[str, Deprecate('')].

I guess it will error right away so not this is not that important

@Viicos
Copy link
Contributor

Viicos commented Nov 26, 2023

So PEP 702 got accepted, and in its final form it does not support deprecating specific typed fields.

Do you think this should still be implemented in annotated-types, or should we wait for a hypothetical Deprecated implemented in Python? I think there are a lot of unsolved issues with this idea (see related discussion), so might be worth having it merged sooner or later.

@Zac-HD
Copy link
Member

Zac-HD commented Nov 26, 2023

I'd prefer not to implement anything in annotated-types; I think that Annotated[T, warnings.deprecated("message")] would be a suitable substitute. The current draft implementation with a closure makes that a little harder to check for than ideal, but it's still feasible.

I've posted a comment on the CPython implementation PR about making this introspection easier.

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

7 participants