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 support for UnionType with Python 3.11 #548

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

Conversation

jonaslb
Copy link

@jonaslb jonaslb commented Feb 4, 2023

Fixes #533, which already had good details and a proposal for a solution, based on using get_args and get_origin from the typing 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 the Union 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 in main.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.

@github-actions
Copy link

github-actions bot commented Feb 4, 2023

📝 Docs preview for commit 1f9dd82 at: https://63ddbef8955c2d354bae0bbd--typertiangolo.netlify.app

@jonaslb
Copy link
Author

jonaslb commented Feb 4, 2023

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.

@github-actions
Copy link

github-actions bot commented Feb 6, 2023

📝 Docs preview for commit 84ad04c at: https://63e1682403f85605237e7c38--typertiangolo.netlify.app

@jonaslb
Copy link
Author

jonaslb commented Feb 6, 2023

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

@austin-silk
Copy link

@tiangolo - can we merge this please?

@jonaslb
Copy link
Author

jonaslb commented Oct 3, 2023

Rebased on master and now all is green 👍

Still happy to make adaptations or answer questions.

@svlandeg svlandeg added feature New feature, enhancement or request types Type hints and type checking p3 labels Mar 1, 2024
@jonaslb
Copy link
Author

jonaslb commented Mar 29, 2024

@svlandeg I gather that #676 is the preferred solution to the UnionType situation? Should I just close this? Will we ever get anything on the topic merged - ie. what's blocking?

@svlandeg
Copy link
Collaborator

svlandeg commented Mar 29, 2024

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.

@jonaslb
Copy link
Author

jonaslb commented Mar 29, 2024

Open-source maintenance costs time and effort as I hope you can appreciate ;-)

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.

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.

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.

@svlandeg svlandeg added bug Something isn't working p2 and removed feature New feature, enhancement or request p3 labels Apr 11, 2024
@svlandeg svlandeg changed the title Fix #533: UnionType handling as with Union for Optional values 🐛 Fix support for UnionType with Python 3.11 Apr 11, 2024
@svlandeg svlandeg linked an issue Apr 11, 2024 that may be closed by this pull request
7 tasks
@svlandeg svlandeg linked an issue Apr 11, 2024 that may be closed by this pull request
7 tasks
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 types Type hints and type checking
Projects
None yet
3 participants