-
-
Notifications
You must be signed in to change notification settings - Fork 127
Support ParamSpec for TypeAliasType #449
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
This comment was marked as off-topic.
This comment was marked as off-topic.
| self.assertEqual(get_args(callable_concat), (Concatenate[int, P],)) | ||
| concat_usage = callable_concat[str] | ||
| self.assertEqual(get_args(concat_usage), ((int, str),)) | ||
| self.assertEqual(concat_usage, callable_concat[[str]]) |
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.
Should this block be removed? This is more about GenericAlias usage here and how Concatenate/Unpack is handled.
I think the self.assertEqual(concat_usage, callable_concat[[str]]) test is interesting, but I am not sure about the specifications if this should be valid and stay here.
See also the test_invalid_cases tests where this is redone with a TypeVar.
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.
Since using types.GenericAlias these tests became much clearer. Given that Concatenate might need some special treatmeant in TypeAliasType.__getitem__ (currently done by isinstance(param, list)) and this is the only test where this is looked into with a bit more detail, I think it should probaly stay.
AlexWaygood
left a comment
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 haven't looked deeply at the changes in this PR yet (will do so either today or tomorrow), but here's some comments from skimming through:
Related to: python/cpython#124787
minor comment corrections
| return self.__name__ | ||
|
|
||
| if sys.version_info < (3, 11): | ||
| def _check_single_param(self, param, recursion=0): |
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.
Doesn't seem like this needs to be a generator, it always yields a single value. Let's make it return that value instead and simplify _check_parameters.
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.
Looking at it again. As the current cpython implementation does not not seems to use _type_convert or any restrictions, should we just drop all checks here?
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.
Changes in #489 are necessary to remove _check_parameters / _type_convert. More specifically, _ConcatenateGenericAlias needs _copy_with and __getitem__ if it is not converted to a list via the above functions to be handled correctly in 3.8 & 3.9.
| with self.subTest("get_args of Concatenate in TypeAliasType"): | ||
| if not TYPING_3_9_0: | ||
| # args are: ([<class 'int'>, ~P2],) | ||
| self.skipTest("Nested ParamSpec is not substituted") |
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.
Fixes #448 that usage of ParamSpec with TypeAliasType was limited for < Python 3.11
This PR enables
TypeAliasTypeto use Ellipsis and list arguments forParamSpectype params.