Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 33.9k
bpo-44490: Add __parameters__ and __getitem__ to types.Union#26980
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 1, 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 1, 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.
@uriyyo thank you for sending this PR so quickly! Excellent work. I only have a few minor readability nits below.
Uh oh!
There was an error while loading. Please reload this page.
Misc/NEWS.d/next/Core and Builtins/2021-07-01-11-59-34.bpo-44490.xY80VR.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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
uriyyo commented Jul 2, 2021
@Fidget-Spinner Thanks for review, I have added tests that you suggested. |
Fidget-Spinner commented Jul 2, 2021
Thanks for applying the suggestions. This LGTM. I've requested a review from Guido and I'm waiting for him to take a look when he has the time. I can't merge PRs, sorry. |
Fidget-Spinner commented Jul 3, 2021
Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>
…90.xY80VR.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>
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 3, 2021
@Fidget-Spinner Thanks, fixed |
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.
LG, I would just update two comments slightly.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>
Fidget-Spinner commented Jul 5, 2021 • 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.
Eventually if this PR is merged, we may have to consider backporting this to 3.10. Since it's a somewhat big change, I'm pinging @pablogsal as the 3.10 RM for his awareness. TLDR: Some uncommon but valid use cases not covered by PEP 604 were discovered in bpo. This PR adds support for them. Can it land in 3.10 beta 4 too? |
pablogsal commented Jul 5, 2021
We are too far in the development cycle to add new features, specially stuff that is touching c code. I do understand that this is something new in 3.10 so we do not have possibility of regressions and I see that this is an important point. I think we are fine to get this in with the following requirements:
|
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.
Thanks!
gvanrossum commented Jul 5, 2021
@Fidget-Spinner Can you think of another core dev to review this (not Pablo)? |
pablogsal commented Jul 5, 2021
I can do a review if you don't find anyone else, but currently, I am flooded with PEP 657 stuff so it will probably be in a couple of days |
bedevere-bot commented Jul 5, 2021
🤖 New build scheduled with the buildbot fleet by @pablogsal for commit 7799a15 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
Fidget-Spinner commented Jul 5, 2021 • 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.
@serhiy-storchaka , are you alright with reviewing this? In the meantime, the Windows 10 buildbot failures seem potentially related so @uriyyo please look into them. I'll take a look too in a day or two. |
pablogsal commented Jul 6, 2021
I am restarting them as I am quite sure this is due to corrupted pyc files from a previous run. New builds here: https://buildbot.python.org/all/#/builders/577/builds/86 |
Fidget-Spinner commented Jul 6, 2021 • 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.
Oh you're right. Sorry @uriyyo if I led you on a wild goose chase. I suspect one of two things
|
Fidget-Spinner commented Jul 6, 2021 • 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.
After spending a few hours investigating this. I think our best bet is to merge latest main into this PR and try running the buildbots again. I have no clue why those 2 buildbots fail and any help would be much appreciated. Some findings:
|
gvanrossum commented Jul 6, 2021
Yeah, merge latest main is definitely a good first step. |
uriyyo commented Jul 6, 2021 • 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.
@gvanrossum@Fidget-Spinner I have merged latest main into PR branch. And I have a question should we fix more cases in the scope of this PR?
>>>typing.List[list[T] |float].__parameters__ ()UPD. Actually, there is a lot of case at |
gvanrossum commented Jul 6, 2021
Tests pass now, I propose to merge now and do another PR for the other issues. We still need another core dev approval, but I'm merging now. ("Merged before b4" was one of the requirements, "two core devs" is the other.) |
uriyyo commented Jul 6, 2021
Great, I will open another PR to cover cases that I mentioned. |
…ugfix backports Co-Authored-By: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com> Co-Authored-By: Guido van Rossum <gvanrossum@gmail.com> Co-Authored-By: Yurii Karabas <1998uriyyo@gmail.com>
…ythonGH-26980) Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com> Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>. (cherry picked from commit c45fa1a) Co-authored-by: Yurii Karabas <1998uriyyo@gmail.com>
serhiy-storchaka commented Jul 17, 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 17, 2021
@serhiy-storchaka Do you need help with it? There is also issue with |
pablogsal commented Jul 17, 2021
Please, see my other comment: #27207 (comment) |
…H-26980) (GH-27207) Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com> Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>. (cherry picked from commit c45fa1a) Co-authored-by: Yurii Karabas <1998uriyyo@gmail.com> Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>
https://bugs.python.org/issue44490