-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
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
Conversation
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 |
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.
This LGTM, with some minor notes about docs below. Thanks for doing this!
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>
Thanks! I applied your suggestions. |
It seems it's fixed on the latest master commits. Try rebasing against new changes in master. |
Just merged master, hopefully everything will be green this time. |
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.
Just little nits.
Doc/library/typing.rst
Outdated
assert get_origin(P.args) is P | ||
assert get_origin(P.kwargs) is P |
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.
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
Lib/test/test_typing.py
Outdated
self.assertEqual(repr(P_co.args), "+P_co.args") | ||
self.assertEqual(repr(P_co.kwargs), "+P_co.kwargs") |
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.
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.
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.
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.
Misc/NEWS.d/next/Library/2021-04-08-19-32-26.bpo-47383.YI1hdL.rst
Outdated
Show resolved
Hide resolved
I'll push the remaining changes manually. Co-authored-by: Guido van Rossum <guido@python.org>
Thanks Guido for the review! I addressed your comments. |
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.
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.
This makes the
.args
and.kwargs
attributes ofParamSpec
objects introspectable at runtime. Before this PR, these attributes are justobject()
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