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 (e.g. str | None) with Python 3.11 #548

Merged
merged 8 commits into from
Aug 17, 2024

Conversation

jonaslb
Copy link
Contributor

@jonaslb jonaslb commented Feb 4, 2023

Potential solution for #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
Contributor 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
Contributor 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
Contributor 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
Contributor 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
Member

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

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
@lachaib
Copy link

lachaib commented Jun 3, 2024

Ruff tries to automatically fix these Optional[str] in my parameters and I had to disable it, so I'm really looking forward to seeing this merged and released.

@luispsantos
Copy link

luispsantos commented Jul 11, 2024

@tiangolo I understand you have other time commitments and it must be overwhelming, but I would like to have this PR merged whenever possible. I use Ruff with pyupgrade rules like @lachaib and I find the lack of the modern union support a considerable hindrance when using Typer and Ruff together.

@jonaslb jonaslb mentioned this pull request Jul 12, 2024
3 tasks
@tiangolo tiangolo changed the title 🐛 Fix support for UnionType with Python 3.11 🐛 Fix support for UnionType (e.g. str | None) with Python 3.11 Aug 17, 2024
@tiangolo
Copy link
Member

Great! Thank you for your contribution @jonaslb! 🍰

And in particular, thank you for your patience and your kindness. 🤗

Thank you @svlandeg for the great job as always investigating, reviewing, and reporting everything. 🙇

This will be available in Typer 0.12.4, released in the next hours. 🎉

@tiangolo tiangolo merged commit 218bf89 into fastapi:master Aug 17, 2024
20 checks passed
@NikosAlexandris
Copy link

NikosAlexandris commented Oct 9, 2024

Does this "fix" not include support for X | Y | Z ?

@jonaslb
Copy link
Contributor Author

jonaslb commented Oct 9, 2024

This was always a fix for X | None only, i.e. #533. It looks like an error that it was not closed yet (likely because I didn't use the magic words in the description..). #461 looks like the issue for what you seek.

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
Development

Successfully merging this pull request may close these issues.

TypeError: unsupported operand type(s) for |: 'type' and 'NoneType' PEP 604 Support
8 participants