Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
bpo-44490: Improve typing module compatibility with types.Union#27048
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
uriyyo commented Jul 6, 2021 • edited by bedevere-bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by bedevere-bot
Uh oh!
There was an error while loading. Please reload this page.
uriyyo commented Jul 6, 2021
@Fidget-Spinner Could you please review this PR? |
Fidget-Spinner commented Jul 6, 2021
The Python typing parts LGTM. But I need to do some revision for the C parts. |
Fidget-Spinner commented Jul 7, 2021
Hmm I wonder if it would be better if you split out the pure Python changes (like the nested My reasoning follows that the Python changes can easily be backported. The C changes not so much and will require much more deliberation. What do you think Guido? @gvanrossum |
gvanrossum commented Jul 7, 2021
Agreed, this currently looks like two independent changes. See also the NEWS file. |
uriyyo commented Jul 7, 2021
@Fidget-Spinner@gvanrossum Thats makes sense. I will convert this PR into containing only |
uriyyo commented Jul 7, 2021
@Fidget-Spinner Could you please review this PR? |
Fidget-Spinner left a 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.
LGTM. Some minor questions and comments below.
Uh oh!
There was an error while loading. Please reload this page.
Misc/NEWS.d/next/Core and Builtins/2021-07-06-22-22-15.bpo-44490.BJxPbZ.rst Outdated Show resolvedHide resolved
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…90.BJxPbZ.rst Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>
Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>
uriyyo commented Jul 12, 2021
@Fidget-Spinner Could you please review this PR? |
Fidget-Spinner left a comment • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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.
Yurii, sorry for the delay. Was a little busy. This LGTM. Thanks for submitting this PR.
BTW, it looks like the C changes to types.Union in the other PR won't make to 3.10 (we missed the beta 4 deadline). So we don't have to worry about backporting anymore.
uriyyo commented Jul 12, 2021
Thanks for review, should we mention those changes at |
Fidget-Spinner commented Jul 12, 2021
Yes I was just thinking about that ;). I recommend we mention in whatsnew for 3.11 that union now supports nested type parameters. |
uriyyo commented Jul 12, 2021
Great, should it be done at separate PR? |
Fidget-Spinner commented Jul 12, 2021
Please do! I don't think the other C changes to fix typing.Annotated are whatsnew worthy. So you can start on it. |
uriyyo commented Jul 14, 2021
@gvanrossum Could you please review this PR? |
gvanrossum left a 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.
Great work! I will merge.
gvanrossum commented Jul 17, 2021
@Fidget-Spinner Do you think this is worth a backport to 3.10? IIUC @serhiy-storchaka is backporting his types.Union improvements. |
Fidget-Spinner commented Jul 17, 2021
I think it's worth a backport but we can't since the PR adding |
gvanrossum commented Jul 17, 2021
Never mind then. I guess that’s fine. |
Fidget-Spinner commented Jul 18, 2021
Thanks to Serhiy's Mass Cleanup, we can now backport this. |
miss-islington commented Jul 18, 2021
Thanks @uriyyo for the PR, and @gvanrossum for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10. |
bedevere-bot commented Jul 18, 2021
GH-27220 is a backport of this pull request to the 3.10 branch. |
…onGH-27048) (cherry picked from commit bf89ff9) Co-authored-by: Yurii Karabas <1998uriyyo@gmail.com>
https://bugs.python.org/issue44490