-
-
Notifications
You must be signed in to change notification settings - Fork 682
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 support for UnionType
(e.g. str | None
) with Python 3.11
#548
Conversation
📝 Docs preview for commit 1f9dd82 at: https://63ddbef8955c2d354bae0bbd--typertiangolo.netlify.app |
It looks like mypy behaves differently in the different tests across versions. 3.6, 3.10 and 3.11 are ok, but 3.7, 3.8 and 3.9 are not. That seems confusing. I can maybe look at it another day again, otherwise if someone can see why it happens let me know. |
📝 Docs preview for commit 84ad04c at: https://63e1682403f85605237e7c38--typertiangolo.netlify.app |
Failures are down to mypy version I think, it works fine here with latest. I see some errors in other places though (unrelated to my changes). Also realized there is already another PR for this issue (just references other issue numbers, so I hadn't seen it): #522 Regardless, hope to see either PR merged. Ping @tiangolo |
@tiangolo - can we merge this please? |
Rebased on master and now all is green 👍 Still happy to make adaptations or answer questions. |
Hi @jonaslb, we've been going through the backlog of PRs to label them and connect related ones. We've already reviewed, adapted and merged a bunch for last week's releases 0.9.1 through 0.11.0. Open-source maintenance costs time and effort as I hope you can appreciate ;-) Thanks for your contributions (and patience) so far, we'll definitely get to this! It's fine to keep this one open in the meantime. |
Of course, and it's great to see that time is now being spent on typer also from the maintainer side ;-) I'm looking forward to engaging about details here if this PR turns out to be the preferred one out of the many submitted on the topic. |
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.
I confirmed that this behaviour works in 3.10 but broke in 3.11, and the unit test introduced in this PR captures the bug nicely.
I updated this PR so that the changes would be more minimal, using the variables from _typing
that were introduced precisely for this type of usage.
This PR now closely resembles #676, though this one captures a few additional cases where __args__
and __origin__
are called directly in the main code. I think it's a good idea to rewrite all of these cases.
As such, I recommend to merge this PR and close #676 (and #739), but I'll leave the final decision with Tiangolo.
Ruff tries to automatically fix these |
UnionType
(e.g. str | None
) with Python 3.11
Does this "fix" not include support for |
Potential solution for #533, which already had good details and a proposal for a solution, based on using
get_args
andget_origin
from thetyping
module instead of the__args__
and__origin__
attributes. So I went and did that.We also needed to actually handle the
UnionType
in addition to theUnion
from before.Now there was a bit of a headache in terms of writing this in a backwards compatible way. What I've done is place the compatibility code into
_compat_utils.py
. But let me know if you'd prefer to keep it inmain.py
.I also added a single test, just an adapted copy of another one with the
x | None
syntax. It is conditioned on Python >= 3.10, so it shouldn't fail on older Pythons.