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 — GH-87390: Add remaining tests for PEP 646 by mrahtz · Pull Request #98267 · python/cpython

Conversation

@mrahtz

@mrahtz mrahtz commented Oct 14, 2022 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

This PR fixes the last few things from the PEP 646 implementation checklist at #87390 (comment). Specifically:

  • Fix a weird hack we used for star-unpacking in test_genericalias.py before the star-unpacking grammar change was merged
  • Add some more substitution tests (eg [tuple[*Ts, T][str, *tuple[int, ...]])
  • Fix some outdated comments
  • Add tests to typing.py for *ed stuff (again, now that the grammar change is merged)
  • Add tests for get_origin on unpacked stuff

A couple notes:

  • If we were being really exhaustive, we would test the cross product of tuple, Tuple and *, Unpack, for a total of 4 versions of each test, but that would be reeeaaally verbose, so I've gone with just testing the 'new-style' cases (* and tuple) and the 'old-style' cases (Unpack and Tuple).
  • get_origin currently gives different behaviour for *tuple[int] and Unpack[tuple[int]]: tuple in the former case, Unpack in the latter. This is a bit inconsistent, but it seems like the least surprising thing from a user's perspective: if I saw Unpack[something], I'd intuitively expected the origin to be Unpack; and if I saw *tuple[something], I'd expect the origin to be tuple.
  • Issue: Add support for PEP 646 #87390

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
mrahtz added 6 commits October 11, 2022 19:56
@mrahtz
PEP 646: Fix final TODOs in test_genericalias.py
10273c6
@mrahtz
PEP 646: Add tests for tuple[*Ts, T][str, *tuple[int, ...]] and similar
5e76ad5
@mrahtz
PEP 646: Fix some comments in test_typing
4b51b13
@mrahtz
Merge duplicate tests with same name
1d35bac
@mrahtz
Add tests using * instead of Unpack
739ccf5
@mrahtz
Add tests for get_origin on starred/unpacked things
35d1a82
@mrahtz
mrahtz requested review from Fidget-Spinner, JelleZijlstra and gvanrossum as code owners October 14, 2022 17:44
@bedevere-bot bedevere-bot added awaiting review tests Tests in the Lib/test dir labels Oct 14, 2022
@AlexWaygood AlexWaygood added the topic-typing label Oct 14, 2022
gvanrossum
gvanrossum reviewed Oct 15, 2022
View reviewed changes
Comment thread Lib/test/test_typing.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
gvanrossum and others added 4 commits October 14, 2022 17:44
@gvanrossum
Remove stray pdb.set_trace() call
f09cce9 
PS. You can use `breakpoint()` instead of that. :-)
@mrahtz
Fix get_origin test and remove debug line
af8b972
@mrahtz
Merge branch 'main' into pep646-cleanups
a48eadc
@blurb-it
📜🤖 Added by blurb_it.
69caf87
@mrahtz

mrahtz commented Oct 15, 2022

Copy link
Copy Markdown
Contributor Author

Oops, good catch - I think I must have accidentally committed my debug changes, because I found some other things that needed removing. Have fixed them and confirmed that CI is actually passing now :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@JelleZijlstra JelleZijlstra added the needs backport to 3.11 only security fixes label Oct 15, 2022
JelleZijlstra
JelleZijlstra reviewed Oct 15, 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! Just a few small things.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_typing.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_typing.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
mrahtz added 2 commits October 16, 2022 09:43
@mrahtz
Move substitution tests into right test case
67dcbfd
@mrahtz
Add test for get_args(*tuple[*Ts])
805713c
gvanrossum
gvanrossum approved these changes Oct 16, 2022
View reviewed changes

@gvanrossum gvanrossum 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.

I'm mostly relying on Jelle's keen eye here. This will have to miss 3.11.0 but will be in 3.11.1.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_typing.py
Comment on lines +595 to +597
For variadic cases, these tests should be regarded as the source of truth,
since we hadn't realised the full complexity of variadic substitution
at the time of finalizing PEP 646. For full discussion, see

@gvanrossum gvanrossum Oct 16, 2022

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.

If the SC doesn't object maybe we could update the PEP with a more exact specification? Since this was discovered during the beta test period it makes sense to me to update the PEP. Storing a part of a specification permanently in an issue seems iffy to me (plenty of tests refer to the issue that caused the test to be added, but the wording here makes me think that the situation here is different -- this is effectively part of the specification).

Sorry, something went wrong.

Uh oh!

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

@mrahtz mrahtz Oct 20, 2022

Copy link
Copy Markdown
Contributor Author

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.

Sure - if the SC is fine with it, I'd be happy to write this up in the PEP properly.

Sorry, something went wrong.

Uh oh!

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

@gvanrossum gvanrossum Oct 24, 2022

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.

@mrahtz From the SC issue I gather the following:

  • We should add these tests (they'll land in 3.11.1, that's fine)
  • We (you) should write the PEP update and submit it to the SC for formal approval

I expect the SC approval for the PEP updates should be easy to obtain.

Sorry, something went wrong.

Uh oh!

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

@mrahtz mrahtz Oct 26, 2022

Copy link
Copy Markdown
Contributor Author

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.

Ok, great. I'll work on the PEP updates as soon as I have some time.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Oct 16, 2022
@gvanrossum

gvanrossum commented Oct 24, 2022

Copy link
Copy Markdown
Member

@JelleZijlstra Are you happy with these tests? Then you can merge them.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@JelleZijlstra
JelleZijlstra self-requested a review October 24, 2022 16:47
@JelleZijlstra

JelleZijlstra commented Oct 24, 2022

Copy link
Copy Markdown
Member

Probably, I'll take another look tonight and merge if I'm happy.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
JelleZijlstra
JelleZijlstra approved these changes Oct 25, 2022
View reviewed changes
@JelleZijlstra JelleZijlstra self-assigned this Oct 25, 2022
@JelleZijlstra
JelleZijlstra merged commit cb95cc2 into python:main Oct 25, 2022
@miss-islington

miss-islington commented Oct 25, 2022

Copy link
Copy Markdown
Contributor

Thanks @mrahtz for the PR, and @JelleZijlstra for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the awaiting merge label Oct 25, 2022
@miss-islington

miss-islington commented Oct 25, 2022

Copy link
Copy Markdown
Contributor

Sorry, @mrahtz and @JelleZijlstra, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker cb95cc24efecf32ad035318867b065a2b042d25a 3.11
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
JelleZijlstra pushed a commit to JelleZijlstra/cpython that referenced this pull request Oct 25, 2022
@mrahtz @JelleZijlstra
[3.11] pythonGH-87390: Add remaining tests for PEP 646 (pythonGH-98267)
fc6f7cf 
Co-authored-by: Guido van Rossum <[email protected]>.
(cherry picked from commit cb95cc2)

Co-authored-by: Matthew Rahtz <[email protected]>
@bedevere-bot

bedevere-bot commented Oct 25, 2022

Copy link
Copy Markdown

GH-98667 is a backport of this pull request to the 3.11 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Oct 25, 2022
@armw4 armw4 mentioned this pull request Oct 25, 2022
[3.11] GH-87390: Add remaining tests for PEP 646 (GH-98267) #98667
Merged
JelleZijlstra added a commit that referenced this pull request Oct 26, 2022
@JelleZijlstra
[3.11] GH-87390: Add remaining tests for PEP 646 (GH-98267) (#98667)
83c8df2 
Co-authored-by: Guido van Rossum <[email protected]>.
Co-authored-by: Matthew Rahtz <[email protected]>

(cherry picked from commit cb95cc2)
@mrahtz mrahtz mentioned this pull request Nov 12, 2022
PEP 646: Add section on type substitution in generic aliases python/peps#2883
Merged
@mrahtz mrahtz mentioned this pull request Apr 10, 2023
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

@gvanrossum gvanrossum gvanrossum approved these changes
@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes
@Fidget-Spinner Fidget-Spinner Awaiting requested review from Fidget-Spinner

Uh oh!

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

Assignees

@JelleZijlstra JelleZijlstra

Labels

tests Tests in the Lib/test dir topic-typing

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.

6 participants

@mrahtz @gvanrossum @JelleZijlstra @miss-islington @bedevere-bot @AlexWaygood
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.