GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-43224: Implement PEP 646 changes to genericaliasobject.c by mrahtz · Pull Request #31019 · python/cpython

Conversation

@mrahtz

@mrahtz mrahtz commented Jan 30, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@mrahtz
PEP 646: Implement changes to genericaliasobject.c
e8bb03b
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Jan 30, 2022
@bedevere-bot bedevere-bot added the awaiting review label Jan 30, 2022
@mrahtz
mrahtz marked this pull request as ready for review January 30, 2022 13:16
@AlexWaygood
AlexWaygood requested a review from Fidget-Spinner January 30, 2022 13:21
JelleZijlstra
JelleZijlstra requested changes Jan 30, 2022
View reviewed changes

@JelleZijlstra JelleZijlstra left a comment

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Objects/genericaliasobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Objects/genericaliasobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Objects/genericaliasobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Objects/genericaliasobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot added awaiting core review and removed awaiting review labels Jan 30, 2022
blurb-it Bot and others added 6 commits January 30, 2022 20:32
@blurb-it
📜🤖 Added by blurb_it.
9cb2aec
@mrahtz @JelleZijlstra
Swap an assignment and an INCREF to a NewRef
b3fa489
Co-authored-by: Jelle Zijlstra <[email protected]>
@mrahtz
Fix cast whitespace
ec81d44
@mrahtz
Add NULL check
968b3fb
@mrahtz
Remove restriction that only tuple can be unpacked
4aa94df
@mrahtz
Remove test which is no longer necessary
c256bc9
mrahtz
mrahtz commented Jan 30, 2022
View reviewed changes
Comment thread Objects/genericaliasobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Objects/genericaliasobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Objects/genericaliasobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Objects/genericaliasobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

kumaraditya303
kumaraditya303 reviewed Jan 31, 2022
View reviewed changes
Comment thread Objects/genericaliasobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

kumaraditya303
kumaraditya303 reviewed Jan 31, 2022
View reviewed changes
Comment thread Objects/genericaliasobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

kumaraditya303
kumaraditya303 reviewed Jan 31, 2022
View reviewed changes
Comment thread Objects/genericaliasobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

mrahtz and others added 4 commits February 1, 2022 15:33
@mrahtz
Fix news
e4a72b4
@mrahtz
Import suggestions from kumaraditya303 from previous PR
742d283
@mrahtz @kumaraditya303
Use public PyObject_GC_Track
d4d97b3
Co-authored-by: Kumar Aditya <[email protected]>
@mrahtz @kumaraditya303
Use public PyObject_GC_UnTrack
7f06e2a
Co-authored-by: Kumar Aditya <[email protected]>
Fidget-Spinner
Fidget-Spinner reviewed Feb 1, 2022
View reviewed changes
Comment thread Objects/genericaliasobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@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 :)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

pablogsal
pablogsal reviewed Feb 1, 2022
View reviewed changes
Comment thread Objects/genericaliasobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

pablogsal
pablogsal reviewed Feb 1, 2022
View reviewed changes
Comment thread Objects/genericaliasobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

pablogsal
pablogsal reviewed Feb 1, 2022
View reviewed changes
Comment thread Objects/genericaliasobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@mrahtz mrahtz mentioned this pull request Feb 2, 2022
bpo-43224: Implement PEP 646 changes to typing.py #31021
Merged
@kumaraditya303
fix gc
6bf3caa
@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mrahtz

mrahtz commented Mar 10, 2022

Copy link
Copy Markdown
Contributor Author

@Fidget-Spinner Done.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

bedevere-bot commented Mar 10, 2022

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

JelleZijlstra commented Mar 10, 2022

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.

AlexWaygood and mrahtz reacted with laugh emoji
All reactions
  • 😄 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@JelleZijlstra

JelleZijlstra commented Mar 10, 2022

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mrahtz
Merge branch 'main' into pep646-tuple
eda60f9
@mrahtz

mrahtz commented Mar 10, 2022

Copy link
Copy Markdown
Contributor Author

That was fast! I've re-merged.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

bedevere-bot commented Mar 10, 2022

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

JelleZijlstra commented Mar 11, 2022

Copy link
Copy Markdown
Member

The refleak buildbots are still unhappy

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@JelleZijlstra

JelleZijlstra commented Mar 11, 2022

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Fidget-Spinner

Fidget-Spinner commented Mar 11, 2022

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mrahtz
Merge branch 'main' into pep646-tuple
5fe058e
@mrahtz

mrahtz commented Mar 11, 2022

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

bedevere-bot commented Mar 11, 2022

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

AlexWaygood commented Mar 11, 2022

Copy link
Copy Markdown
Member

Looks like the buildbots are finally happy! 🎉

kumaraditya303 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Fidget-Spinner
Fidget-Spinner approved these changes Mar 11, 2022
View reviewed changes

@Fidget-Spinner Fidget-Spinner left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gvanrossum reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@Fidget-Spinner
Fidget-Spinner merged commit af2277e into python:main Mar 12, 2022
@bedevere-bot bedevere-bot removed the awaiting merge label Mar 12, 2022
@Fidget-Spinner

Fidget-Spinner commented Mar 12, 2022

Copy link
Copy Markdown
Member

Congrats @mrahtz !

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mrahtz

mrahtz commented Mar 13, 2022

Copy link
Copy Markdown
Contributor Author

Fantastic - thanks @Fidget-Spinner!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mrahtz
mrahtz deleted the pep646-tuple branch March 13, 2022 10:14
@mrahtz mrahtz mannequin mentioned this pull request Apr 11, 2022
Add support for PEP 646 #87390
Closed
@AlexWaygood AlexWaygood mentioned this pull request Apr 11, 2023
All instances of types.GenericAlias are iterable on Python 3.11+ #103450
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pablogsal pablogsal pablogsal left review comments
@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes
@Fidget-Spinner Fidget-Spinner Fidget-Spinner approved these changes
@kumaraditya303 kumaraditya303 kumaraditya303 approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@mrahtz @Fidget-Spinner @bedevere-bot @JelleZijlstra @AlexWaygood @pablogsal @kumaraditya303 @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.