Skip to content

Conversation

@horta
Copy link
Contributor

@horta horta commented Nov 12, 2024

@horta horta force-pushed the avoid-typing-extensions branch from 08f1d06 to 176a06e Compare November 12, 2024 14:52
@horta horta force-pushed the avoid-typing-extensions branch from 176a06e to ec22523 Compare November 12, 2024 15:00
@svlandeg svlandeg changed the title Avoid the unnecessary import of typing_extensions ✨ Avoid the unnecessary import of typing_extensions in newer Python versions Nov 13, 2024
@svlandeg svlandeg added the feature New feature, enhancement or request label Nov 13, 2024
Copy link
Member

@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! This makes sense. We do have the check in other places where we're importing things like Literal, though we're checking on v3.8 in those instances. From the link you shared, I see that typing_extensions has an updated definition for 3.8, so it makes sense to only import from typing from 3.9 onwards. Perhaps we should even make that change across the codebase to be consistent.

@horta
Copy link
Contributor Author

horta commented Nov 13, 2024

I dont understand that linting error for the rich_utils.py file. If I import Literal directly from typing_extensions or typing, it works, but not if I import it from _typing?

@svlandeg svlandeg self-assigned this Nov 14, 2024
@svlandeg
Copy link
Member

Hm, that's weird. I can have a look sometime later this week if you don't figure it out!

@svlandeg svlandeg marked this pull request as draft November 14, 2024 11:04
@svlandeg
Copy link
Member

svlandeg commented Dec 26, 2024

I dont understand that linting error for the rich_utils.py file. If I import Literal directly from typing_extensions or typing, it works, but not if I import it from _typing?

Looks like this is expected behaviour when importing Typing from an indirect import 😕 : astral-sh/ruff#128 (comment)

Meh - I guess we'll just repeat the import code, it's not that big a deal.

@svlandeg svlandeg marked this pull request as ready for review December 26, 2024 09:56
@svlandeg svlandeg removed their assignment Dec 26, 2024
Copy link
Member

@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.

Should be good to merge!

Copy link
Member

@tiangolo tiangolo left a comment

Choose a reason for hiding this comment

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

Nice, thank you! ☕ 🍰

@tiangolo tiangolo merged commit ab52638 into fastapi:master Feb 27, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New feature, enhancement or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants