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: Work around substitution of unpacked TypeVarTuple by mrahtz · Pull Request #31804 · python/cpython

Conversation

@mrahtz

@mrahtz mrahtz commented Mar 10, 2022 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

Following up on the discussion in #31021 about how to handle substitution of TypeVarTuples, here's a first attempt at the solution Jelle suggested: when TypeVarTuples` are involved, don't perform the substitution at all, instead returning an object whose repr() reflects that.

(What Jelle actually suggested was to return a new _GenericAlias here. I've tested that too, and it seems to work fine, but I'm a bit wary about giving _GenericAlias too many hats to wear - as a newcomer to this codebase, I've struggled with how overloaded classes already are, making it predict what the effects of a change will be. Having said that, Jelle, I remember you mentioning you'd prefer to keep the number of classes in typing.py small, so if you think the tradeoff isn't worth it, I respect your judgement, and we can re-use _GenericAlias, coping with the extra functionality with extra documentation comments.)

The main alternative I'm aware of is the logic we cut out in b9b1c80 - about 200 lines of logic, and another 100 lines of tests. @serhiy-storchaka also has a partial solution in #31800, which could also be an option - making the substitutions only when it's easy, and otherwise falling back to the approach in this PR.

I've also pulled in @serhiy-storchaka's nice assertEndsWith helper from #31800 :)

@JelleZijlstra @pradeep90

https://bugs.python.org/issue43224

Sorry, something went wrong.

Uh oh!

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

All reactions
@mrahtz
Work around substitution of unpacked TypeVarTuple
9b01680
@mrahtz
mrahtz requested review from Fidget-Spinner, JelleZijlstra and gvanrossum as code owners March 10, 2022 22:51
@bedevere-bot bedevere-bot added the awaiting review label Mar 10, 2022
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Mar 10, 2022
JelleZijlstra
JelleZijlstra reviewed Mar 11, 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! I think it's better to go with _GenericAlias, because otherwise we have to re-implement a lot of what that class does: pickle support, __eq__, __iter__.

The idea would be that this is a step towards a rule where we can say X[T] == _GenericAlias(X, (T,)), whatever X is.

Sorry, something went wrong.

Uh oh!

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

All reactions
@mrahtz mrahtz mentioned this pull request Mar 12, 2022
bpo-43224: Implement substitution of unpacked TypeVarTuple #31800
Merged
@mrahtz mrahtz closed this Mar 13, 2022
@mrahtz
mrahtz deleted the pep646-typevartuple-subst branch March 13, 2022 10:14
@mrahtz

mrahtz commented Mar 13, 2022

Copy link
Copy Markdown
Contributor Author

(Closed since it looks like @serhiy-storchaka is insisting on going ahead with #31800.)

All reactions

Sorry, something went wrong.

Uh oh!

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

@mrahtz mrahtz mannequin mentioned this pull request Apr 11, 2022
Add support for PEP 646 #87390
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JelleZijlstra JelleZijlstra JelleZijlstra left review comments
@gvanrossum gvanrossum Awaiting requested review from gvanrossum
@Fidget-Spinner Fidget-Spinner Awaiting requested review from Fidget-Spinner

Uh oh!

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

Assignees

No one assigned

Labels

awaiting review

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.

4 participants

@mrahtz @JelleZijlstra @the-knights-who-say-ni @bedevere-bot
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.