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

🐛 Fix usage of Annotated with future annotations in Python 3.8 #814

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ivantodorovich
Copy link
Contributor

@ivantodorovich ivantodorovich commented Apr 30, 2024

Fixes #802

RuntimeError: Type not yet supported: typing_extensions.Annotated[...]

The solution is to use the get_type_hints method from typing_extensions instead of the built-in in the typing module, as it includes backports that support properly evaluating the forward reference.

Fixes tiangolo#802

`RuntimeError: Type not yet supported: typing_extensions.Annotated[...]`

The solution is to use the `get_type_hints` method from `typing_extensions`
instead of the built-in in the `typing` module, as it includes backports that
support properly evaluating the forward reference.
@ivantodorovich ivantodorovich marked this pull request as ready for review April 30, 2024 11:37
@svlandeg svlandeg added bug Something isn't working p2 labels May 2, 2024
@kinuax
Copy link

kinuax commented May 12, 2024

PR Review

I checked the changes locally on Linux and all tests pass on 3.8..3.12. I reverted the bugfix, run all tests again on 3.8..3.12 and test_future_annotations.py fails on 3.8 as expected:

RuntimeError: Type not yet supported: typing_extensions.Annotated[bool, <typer.models.OptionInfo object>]

Copy link
Collaborator

@svlandeg svlandeg left a comment

Choose a reason for hiding this comment

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

Thanks for the PR - great catch and fix!

I confirmed locally that the behaviour is broken with Python 3.8 and fixed with this PR.

@ivantodorovich: as a "pro tip", next time you create a PR to resolve a bug, you could consider committing the failing test separately as the first commit. This will run the CI, which will be red, clearly showcasing the problem. Then you'd push your fix, the CI would be rerun and would be green, demonstrating that you've fixed things 😉

One final comment for Tiangolo: not sure whether you want this new test in the tests root or perhaps some place else like test_compat - I'll leave it like this for now for you to decide.

@ivantodorovich
Copy link
Contributor Author

@ivantodorovich: as a "pro tip", next time you create a PR to resolve a bug, you could consider committing the failing test separately as the first commit. This will run the CI, which will be red, clearly showcasing the problem. Then you'd push your fix, the CI would be rerun and would be green, demonstrating that you've fixed things 😉

Good tip! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants