Skip to content

bpo-43224: Implement PEP 646 changes to genericaliasobject.c#31019

Merged
Fidget-Spinner merged 19 commits into
python:mainfrom
mrahtz:pep646-tuple
Mar 12, 2022
Merged

bpo-43224: Implement PEP 646 changes to genericaliasobject.c#31019
Fidget-Spinner merged 19 commits into
python:mainfrom
mrahtz:pep646-tuple

Conversation

@mrahtz

@mrahtz mrahtz commented Jan 30, 2022

Copy link
Copy Markdown
Contributor

These are the parts of the old PR (#30398) relevant to starring tuples. This allows us to do things like *args: *tuple[int, *Ts] (where Ts is a TypeVarTuple).

@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 starred typing.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 to typing.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

@mrahtz mrahtz marked this pull request as ready for review January 30, 2022 13:16

@JelleZijlstra JelleZijlstra left a comment

Copy link
Copy Markdown
Member

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

Comment thread Objects/genericaliasobject.c Outdated
Comment thread Objects/genericaliasobject.c Outdated
Comment thread Objects/genericaliasobject.c Outdated
Comment thread Objects/genericaliasobject.c Outdated
Comment thread Objects/genericaliasobject.c
Comment thread Objects/genericaliasobject.c Outdated
Comment thread Objects/genericaliasobject.c Outdated
Comment thread Objects/genericaliasobject.c Outdated
Comment thread Objects/genericaliasobject.c Outdated
Comment thread Objects/genericaliasobject.c Outdated
Comment thread Objects/genericaliasobject.c Outdated
mrahtz and others added 4 commits February 1, 2022 15:33
Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
Comment thread Objects/genericaliasobject.c
@mrahtz

mrahtz commented Feb 1, 2022

Copy link
Copy Markdown
Contributor Author

@pablogsal Might be good to have your eyes on this too, to make sure we get GC correct :)

Comment thread Objects/genericaliasobject.c
Comment thread Objects/genericaliasobject.c Outdated
Comment thread Objects/genericaliasobject.c
@mrahtz

mrahtz commented Feb 14, 2022

Copy link
Copy Markdown
Contributor Author

Apologies for the slow reply - it's been a busy couple of weeks! Taking a look at this feedback now.

@mrahtz

mrahtz commented Mar 10, 2022

Copy link
Copy Markdown
Contributor Author

@Fidget-Spinner Done.

@JelleZijlstra JelleZijlstra added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Mar 10, 2022
@bedevere-bot

Copy link
Copy Markdown

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Mar 10, 2022
@JelleZijlstra

Copy link
Copy Markdown
Member

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.

@JelleZijlstra

Copy link
Copy Markdown
Member

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.

@mrahtz

mrahtz commented Mar 10, 2022

Copy link
Copy Markdown
Contributor Author

That was fast! I've re-merged.

@JelleZijlstra JelleZijlstra added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Mar 10, 2022
@bedevere-bot

Copy link
Copy Markdown

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Mar 10, 2022
@JelleZijlstra

Copy link
Copy Markdown
Member

The refleak buildbots are still unhappy

@JelleZijlstra

Copy link
Copy Markdown
Member

Seems like there's a refleak on main. I'll try to find it.

@Fidget-Spinner

Copy link
Copy Markdown
Member

@mrahtz I just merged the fix for the refleak into main. Can I trouble you to merge main into this PR again please?

@mrahtz

mrahtz commented Mar 11, 2022

Copy link
Copy Markdown
Contributor Author

@Fidget-Spinner Wow, that was also fast :) Done.

@AlexWaygood AlexWaygood added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Mar 11, 2022
@bedevere-bot

Copy link
Copy Markdown

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Mar 11, 2022
@AlexWaygood

Copy link
Copy Markdown
Member

Looks like the buildbots are finally happy! 🎉

@Fidget-Spinner Fidget-Spinner left a comment

Copy link
Copy Markdown
Member

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.

@Fidget-Spinner Fidget-Spinner merged commit af2277e into python:main Mar 12, 2022
@Fidget-Spinner

Copy link
Copy Markdown
Member

Congrats @mrahtz !

@mrahtz

mrahtz commented Mar 13, 2022

Copy link
Copy Markdown
Contributor Author

Fantastic - thanks @Fidget-Spinner!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants