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.

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

backport ParamSpecArgs/Kwargs #798

Merged
merged 7 commits into from
Apr 13, 2021

Conversation

JelleZijlstra
Copy link
Member

From python/cpython#25298. I also added more tests for get_args/get_origin,
which previously didn't exist in in typing_extensions.

From python/cpython#25298. I also added more tests for get_args/get_origin,
which previously didn't exist in in typing_extensions.
@JelleZijlstra
Copy link
Member Author

(Pretty likely that this broke tests in some Python version, I'll monitor CI.)

@JelleZijlstra JelleZijlstra marked this pull request as draft April 11, 2021 03:45
@JelleZijlstra JelleZijlstra marked this pull request as ready for review April 11, 2021 15:25
@JelleZijlstra
Copy link
Member Author

Finally, all the tests pass. @Fidget-Spinner would you mind reviewing this?

@Fidget-Spinner
Copy link
Member

Finally, all the tests pass. @Fidget-Spinner would you mind reviewing this?

Sure, give me some time, I'll try to review it tomorrow (your time).

@JelleZijlstra
Copy link
Member Author

Thanks, no hurry!

@JelleZijlstra JelleZijlstra mentioned this pull request Apr 11, 2021
Copy link
Member

@Fidget-Spinner Fidget-Spinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Thanks for spending time on this.

self.assertEqual(get_args(list), ())
if sys.version_info >= (3, 9):
# Support Python versions with and without the fix for
# https://bugs.python.org/issue42195
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Off topic: I didn't understand these tests at first, but then I saw assertIn and the lists/tuples, and now I find them really smart :).

Also I can't wait for Python 3.14 or higher (however many years down the road that is) where we can drop 3.9 support.

I think it's worth to add a small comment saying what versions this affect and which results are expected, so in the future people don't need to scroll through that really log bpo. Maybe

# The first result is expected in Python 3.9.0/1, the second is valid from 3.9.2 onwards

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, the main reason I didn't do that is that then I have to figure out the exact Python versions each variant supports :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hehe. touché - I would've done the same if I couldn't remember.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some comments here

@@ -2058,11 +2058,23 @@ class Annotated(metaclass=AnnotatedMeta):

# Python 3.8 has get_origin() and get_args() but those implementations aren't
# Annotated-aware, so we can't use those, only Python 3.9 versions will do.
if sys.version_info[:2] >= (3, 9):
# Similarly, Python 3.9's implementation doesn't support ParamSpecArgs and
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another off topic comment: Something I just realised: if ParamSpecArgs/Kwargs had inherited from _GenericAlias in the original implementation, we wouldn't need this. Do you think it's worth it to do that in CPython or not worth the extra complexity/feel it's too hacky and not really what _GenericAlias actually represents?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's definitely an option. I feel like it's too different though. A ParamSpec isn't the type while the origin of a GenericAlias is, and ParamSpecArgs/Kwargs don't really have args that are comparable to those of a GenericAlias.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm yeah. Since ParamSpec isn't a Generic to begin with, GenericAlias definitely feels wrong.

@Fidget-Spinner
Copy link
Member

I'm happy with this. If you're satisfied too, I think you can merge.

@JelleZijlstra JelleZijlstra merged commit 4ba98e8 into python:master Apr 13, 2021
@JelleZijlstra JelleZijlstra deleted the paramspecargs branch April 13, 2021 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants