8000 bpo-43783: add ParamSpecArgs/Kwargs by JelleZijlstra · Pull Request #25298 · python/cpython · GitHub
[go: up one dir, main page]

Skip to content

bpo-43783: add ParamSpecArgs/ 8000 Kwargs #25298

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

Merged
merged 8 commits into from
Apr 11, 2021

Conversation

JelleZijlstra
Copy link
Member
@JelleZijlstra JelleZijlstra commented Apr 9, 2021

This makes the .args and .kwargs attributes of ParamSpec objects introspectable at runtime. Before this PR, these attributes are just object() instances. This arose out of discussion here.

I chose to create two new classes, instead of a single class with an attribute set to "args" or "kwargs", because args and kwargs are the only possible attributes. It feels cleaner to make illegal cases impossible to represent than to use a magical string attribute.

https://bugs.python.org/issue43783

@JelleZijlstra
Copy link
Member Author

cc @Fidget-Spinner

@JelleZijlstra
Copy link
Member Author

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.

This LGTM, with some minor notes about docs below. Thanks for doing this!

JelleZijlstra and others added 4 commits April 9, 2021 07:24
Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>
Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>
Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>
Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>
@JelleZijlstra
Copy link
Member Author

Thanks! I applied your suggestions.

@Fidget-Spinner
Copy link
Member

The CI failure looks unrelated to my changes: https://dev.azure.com/Python/cpython/_build/results?buildId=78089&view=logs&j=4db1505a-29e5-5cc0-240b-53a8a2681f75&t=a975920c-8356-5388-147c-613d5fab0171

It seems it's fixed on the latest master commits. Try rebasing against new changes in master.

@JelleZijlstra
Copy link
Member Author

Just merged master, hopefully everything will be green this time.

@gvanrossum gvanrossum removed the request for review from ilevkivskyi April 10, 2021 22:17
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.

Just little nits.

Comment on lines 1095 to 1096
assert get_origin(P.args) is P
assert get_origin(P.kwargs) is P
Copy link
Member

Choose a reason for hiding this comment

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

While I know the idiom is commonly used and technically correct, formulating an equivalence using an assert does not feel clear to me. I'd much rather show it in a REPL fragment, state the equivalency in English, or show it in a comment, e.g.

get_origin(P.args)  # returns P

Comment on lines 4283 to 4284
self.assertEqual(repr(P_co.args), "+P_co.args")
self.assertEqual(repr(P_co.kwargs), "+P_co.kwargs")
Copy link
Member

Choose a reason for hiding this comment

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

Oh, I didn't realize ParamSpec uses + to show it's covariant. It's clever, although a bit of an innovation for an edge case. It suddenly strikes me as slightly inappropriate here, since this is parsed as +(P_co.args) whereas the meaning is closer to (+P_co).args. But I wouldn't want to introduce those parentheses. Perhaps the variance of the args or kwargs attribute doesn't matter, and the repr() should not include the ~, + or - sign? I don't think this applies for TypeVar, since it has no attributes.

Copy link
Member Author

Choose a reason for hiding this comment

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

It does look a bit weird here, I'll change the repr()s to include only the name. The variance of ParamSpecs has no defined meaning anyway, and even if it did it's not essential for it to be reflected here.

JelleZijlstra and others added 2 commits April 10, 2021 17:48
I'll push the remaining changes manually.

Co-authored-by: Guido van Rossum <guido@python.org>
@JelleZijlstra
Copy link
Member Author

Thanks Guido for the review! I addressed your comments.

@gvanrossum gvanrossum merged commit 5224336 into python:master Apr 11, 2021
@JelleZijlstra JelleZijlstra deleted the paramspecargs branch April 11, 2021 02:58
JelleZijlstra added a commit to JelleZijlstra/typing that referenced this pull request Apr 11, 2021
From python/cpython#25298. I also added more tests for get_args/get_origin,
which previously didn't exist in in typing_extensions.
JelleZijlstra added a commit to python/typing that referenced this pull request Apr 13, 2021
From python/cpython#25298. I also added more tests for get_args/get_origin,
which previously didn't exist in in typing_extensions.
JelleZijlstra added a commit to python/typing_extensions that referenced this pull request May 19, 2022
From python/cpython#25298. I also added more tests for get_args/get_origin,
which previously didn't exist in in typing_extensions.
JelleZijlstra added a commit to python/typing_extensions that referenced this pull request May 19, 2022
From python/cpython#25298. I also added more tests for get_args/get_origin,
which previously didn't exist in in typing_extensions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants
0