bpo-44490: Add __parameters__ and __getitem__ to types.Union#26980
Conversation
Fidget-Spinner
left a comment
There was a problem hiding this comment.
@uriyyo thank you for sending this PR so quickly! Excellent work. I only have a few minor readability nits below.
Sorry, something went wrong.
|
@Fidget-Spinner Thanks for review, I have added tests that you suggested. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
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>
gvanrossum
left a comment
There was a problem hiding this comment.
LG, I would just update two comments slightly.
Sorry, something went wrong.
Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>
|
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? |
Sorry, something went wrong.
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:
|
Sorry, something went wrong.
gvanrossum
left a comment
There was a problem hiding this comment.
Thanks!
Sorry, something went wrong.
|
@Fidget-Spinner Can you think of another core dev to review this (not Pablo)? |
Sorry, something went wrong.
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 |
Sorry, something went wrong.
|
🤖 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. |
Sorry, something went wrong.
@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. |
Sorry, something went wrong.
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 |
Sorry, something went wrong.
Oh you're right. Sorry @uriyyo if I led you on a wild goose chase. I suspect one of two things
|
Sorry, something went wrong.
|
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:
|
Sorry, something went wrong.
|
Yeah, merge latest main is definitely a good first step. |
Sorry, something went wrong.
|
@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 |
Sorry, something went wrong.
|
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.) |
Sorry, something went wrong.
|
Great, I will open another PR to cover cases that I mentioned. |
Sorry, something went wrong.
…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 Do you need help with it? There is also issue with |
Sorry, something went wrong.
Please, see my other comment: #27207 (comment) |
Sorry, something went wrong.
…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