-
-
Notifications
You must be signed in to change notification settings - Fork 7.7k
✨ Add support for PEP695 TypeAliasType #11140
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
Conversation
Kind of unhappy with the amount of hoopsI jump through to not let coverage/pytest really see the test code, as it is a true SyntaxError in all python Version before 3.12. Maybe there is a better idea? |
if sys.version_info < (3, 12): | ||
collect_ignore_glob = ["*_py312.py"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we add that to the previous versions too?
It's a shame to see this PR being ignored for so long, seems like a useful feature to support. |
@@ -325,6 +331,9 @@ def analyze_param( | |||
depends = None | |||
type_annotation: Any = Any | |||
use_annotation: Any = Any | |||
if TypeAliasType is not None and isinstance(annotation, TypeAliasType): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not going to work as expected for all cases; see litestar-org/litestar#3982 (comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a unit test we can design that would exhibit the non-working behaviour for the edge cases you mention?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At Pydantic, we are currently working on a third-party library to handle all these runtime inspection utilities, as per beartype/beartype#479 (comment). The package is ready for it's first release, we just need to setup the documentation infrastructure so it should be available this week.
In the meanwhile, the Pydantic implementation can be used (we have unit tests for the mentioned edge cases).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@svlandeg: we just created the typing-inspection package (docs), currently including the utility functions to inspect typing annotations (type hints evaluation isn't included for now due to the challenges mentioned).
I created a discussion on the Discourse forum to have a single place where things can be discussed. Alternatively, discussion can happen on the Github repository.
cc @tiangolo
@@ -64,6 +64,12 @@ | |||
from starlette.websockets import WebSocket | |||
from typing_extensions import Annotated, get_args, get_origin | |||
|
|||
try: | |||
from typing_extensions import TypeAliasType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typing_extensions.TypeAliasType
was added in 4.6.0, but 4.8.0 is required:
Lines 45 to 48 in ae724b0
dependencies = [ | |
"starlette>=0.40.0,<0.46.0", | |
"pydantic>=1.7.4,!=1.8,!=1.8.1,!=2.0.0,!=2.0.1,!=2.1.0,<3.0.0", | |
"typing-extensions>=4.8.0", |
📝 Docs preview for commit 8dc8298 at: https://d462d901.fastapitiangolo.pages.dev |
Hi @dev-loki, thanks for this contribution, and apologies for the delay in reviewing! There is quite a backlog of PR's to work on, and we're currently trying to manage & prioritize the queue more efficiently. Thanks for your patience! @UltimateLobster: yes - a review by other users (even when they are not maintainers) is always useful. Feel free to be as verbose & nitpicking as you want. For instance, you can be very explicit about which parts you're happy with, which parts you're unsure, what you think is important to change or perhaps some fixes where you note that they are a matter of preference and not necessary. It's always easier for us if you're more verbose, so we understand exactly how you reviewed and what you think of the proposed changes. In general, helping with checking that the PR is up-to-date, passes the CI, is consistent with the rest of the code base, and contains the correct unit checks, is really helpful too. @Viicos: thanks for the review! |
📝 Docs preview for commit 4b06230 at: https://376f8223.fastapitiangolo.pages.dev |
@dev-loki, thank you for your efforts! Could you please take a look at failed tests and address this comment? |
In case @dev-loki is busy or not interested in this PR anymore, does anybody else wants to take this over? |
@YuriiMotov if someone else has to take this over I could do it. I would love to see this land! |
1 similar comment
This comment was marked as duplicate.
This comment was marked as duplicate.
Yes feel free to do it :). I don't have time anymore and also lost context after that time (since then I switched jobs, living places and got another daughter going to school :D) |
@dev-loki thank you for your work! I'll try to base my continued work on your commits to keep the history. Hope your life changes have been good! |
Oh yes they have. In every single aspect :). Thank you for that and for your time form open source
|
Fixes: #10719
Discussed (by others) in: #10662
Adds support for PEP695.
Sidesupport (please check if intended!): Only loads
*_py312
files for python3.12 and later, as only the test introduces syntax not compatible for python < 3.12.Using
DependedVariable
in route dependencies will now work :).