Skip to content
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.

Sign up for GitHub

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

Jump to bottom

GH-87390: Add remaining tests for PEP 646 #98267

Merged
merged 12 commits into from Oct 25, 2022
Merged

GH-87390: Add remaining tests for PEP 646 #98267

merged 12 commits into from Oct 25, 2022

Conversation

mrahtz
Copy link
Contributor

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

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.

@bedevere-bot bedevere-bot added awaiting review tests Tests in the Lib/test dir labels Oct 14, 2022
gvanrossum
gvanrossum reviewed Oct 15, 2022
View reviewed changes
Lib/test/test_typing.py Outdated Show resolved Hide resolved
@mrahtz
Copy link
Contributor Author

mrahtz commented Oct 15, 2022

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

@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
Copy link
Member

@JelleZijlstra JelleZijlstra left a 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.

Lib/test/test_typing.py Outdated Show resolved Hide resolved
Lib/test/test_typing.py Show resolved Hide resolved
gvanrossum
gvanrossum approved these changes Oct 16, 2022
View reviewed changes
Copy link
Member

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

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
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

@gvanrossum
Copy link
Member

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

@JelleZijlstra JelleZijlstra self-requested a review October 24, 2022 16:47
@JelleZijlstra
Copy link
Member

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

@JelleZijlstra JelleZijlstra self-assigned this Oct 25, 2022
@JelleZijlstra JelleZijlstra merged commit cb95cc2 into python:main Oct 25, 2022
@miss-islington
Copy link
Contributor

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

@miss-islington
Copy link
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

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 <gvanrossum@gmail.com>.
(cherry picked from commit cb95cc2)

Co-authored-by: Matthew Rahtz <matthew.rahtz@gmail.com>
@bedevere-bot
Copy link

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

@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
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 <gvanrossum@gmail.com>.
Co-authored-by: Matthew Rahtz <matthew.rahtz@gmail.com>

(cherry picked from commit cb95cc2)
@mrahtz mrahtz mentioned this pull request Nov 12, 2022
Merged
@mrahtz mrahtz mentioned this pull request Apr 10, 2023
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

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.

None yet
6 participants
@mrahtz @gvanrossum @JelleZijlstra @miss-islington @bedevere-bot @AlexWaygood