-
-
Notifications
You must be signed in to change notification settings - Fork 31.4k
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
bpo-43224: Implement PEP 646 changes to genericaliasobject.c #31019
Conversation
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 for splitting up the PR, I think this will be easier to review.
I think you should add a separate news entry for each PR, something like "Allow unpacking GenericAlias objects. Part of PEP 646."
Co-authored-by: Kumar Aditya <[email protected]>
Co-authored-by: Kumar Aditya <[email protected]>
@pablogsal Might be good to have your eyes on this too, to make sure we get GC correct :) |
Apologies for the slow reply - it's been a busy couple of weeks! Taking a look at this feedback now. |
@Fidget-Spinner Done. |
🤖 New build scheduled with the buildbot fleet by @JelleZijlstra for commit 1102d27 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
Now it's failing tests because of #31801, sorry for that! Once that's merged you'll have to merge main into your branch again. |
Main is unbroken now, but I'll see if the current refleak builds say anything; maybe they still tell us something useful even if test_asyncio fails. |
That was fast! I've re-merged. |
🤖 New build scheduled with the buildbot fleet by @JelleZijlstra for commit eda60f9 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
The refleak buildbots are still unhappy |
Seems like there's a refleak on main. I'll try to find it. |
@mrahtz I just merged the fix for the refleak into main. Can I trouble you to merge main into this PR again please? |
@Fidget-Spinner Wow, that was also fast :) Done. |
🤖 New build scheduled with the buildbot fleet by @AlexWaygood for commit 5fe058e 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
Looks like the buildbots are finally happy! 🎉 |
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.
I hereby use the non-existent powers vested in me to press the big green button. Thank you Mathew. This was admittedly very tricky.
I will leave this for at least 24 hours for anyone else to give their feedback before I merge.
Congrats @mrahtz ! |
Fantastic - thanks @Fidget-Spinner! |
These are the parts of the old PR (#30398) relevant to starring tuples. This allows us to do things like
*args: *tuple[int, *Ts]
(whereTs
is aTypeVarTuple
).@gvanrossum Pradeep and I were wondering what the best way of representing a starred
tuple
would be, given that we'll also need a representation of a starredtyping.Tuple
for backport purposes. Should we try and make both*tuple[int]
and*Tuple[int]
refer to the same thing (I guess something we'd add totyping.py
), or is it ok to have them refer to different things (as this PR currently assumes, creating a new native type for*tuple
)?Until the PR with the grammar changes has been merged (#31018), the tests for this look a little awkward. Happy to wait until that PR has been merged to be 100% sure this is correct.
@Fidget-Spinner Would you be happy to review this?
(Also, could someone add the 'skip news' label? The news item is being added in the grammar PR.)Jelle suggests creating a separate news item for each PR, so I've added an item for this one.https://bugs.python.org/issue43224