gh-112319: Allow special protocol members#112340
gh-112319: Allow special protocol members#112340randolf-scholz wants to merge 11 commits intopython:mainfrom
Conversation
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
| cls.__protocol_attrs__ = ( | ||
| _get_local_protocol_members(namespace) | _get_parent_members(cls) | ||
| ) | ||
| # local_attrs = _get_local_protocol_members(namespace) | ||
| # cls.__protocol_attrs__ = local_attrs.union(*map(_get_cls_members, bases)) |
There was a problem hiding this comment.
Not sure which variant is better...
There was a problem hiding this comment.
The only difference between _get_cls_members and _get_parent_members is that the former checks cls.__mro__[:-1] and the latter only cls.__mro__[1:-1].
I would expect the _get_parent_members to be better for deeply inherited classes, and the _get_cls_members to be better for classes with many disjoint parents.
AlexWaygood
left a comment
There was a problem hiding this comment.
haven't looked deeply yet, just a few comments from an initial skim:
| '__init__', '__module__', '__new__', '__slots__', | ||
| '__subclasshook__', '__weakref__', '__class_getitem__', | ||
| '__match_args__', | ||
| '__module__', '__slots__', '__match_args__', '__qualname__', | ||
| }) | ||
| _SPECIAL_CALLABLE_NAMES = frozenset({ | ||
| '__init__', '__new__', '__subclasshook__','__class_getitem__', '__weakref__', | ||
| }) | ||
|
|
||
| # These special attributes will be not collected as protocol members. | ||
| EXCLUDED_ATTRIBUTES = _TYPING_INTERNALS | _SPECIAL_NAMES | {'_MutableMapping__marker'} | ||
| EXCLUDED_MEMBERS = EXCLUDED_ATTRIBUTES | _SPECIAL_CALLABLE_NAMES |
There was a problem hiding this comment.
Please keep the diff as minimal as possible. As far as I can tell, splitting the _SPECIAL_NAMES frozenset into a _SPECIAL_NAMES set and a _SPECIAL_CALLABLE_NAMES set shouldn't make any difference here
There was a problem hiding this comment.
For my solution to work, I need both, because for things defined directly inside the protocol body, I only exclude EXCLUDED_ATTRIBUTES, but not _SPECIAL_CALLABLE_NAMES. (Hence, we can test for __class_getitem__). However, for general classes we need to exclude it (otherwise test.test_typing.ProtocolTests.test_collections_protocols_allowed fails).
See the difference in function body between _get_local_members and _get_local_protocol_members.
Co-authored-by: Alex Waygood <[email protected]>
This comment was marked as duplicate.
This comment was marked as duplicate.
Co-authored-by: Alex Waygood <[email protected]>
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
Co-authored-by: Alex Waygood <[email protected]>
This comment was marked as duplicate.
This comment was marked as duplicate.
Co-authored-by: Alex Waygood <[email protected]>
This comment was marked as resolved.
This comment was marked as resolved.
| return attrs | ||
|
|
||
|
|
||
| def _get_protocol_attrs(cls): |
There was a problem hiding this comment.
After the modifications here, _get_protocol_attrs is not used anymore, and it will even give some wrong results (e.g. if the Protocol defines __class_getitem__). So maybe just delete it?
There was a problem hiding this comment.
If it's not used anywhere anymore, then yes, it should be deleted. It's an undocumented, private function; if anybody's using it and their code is broken by us deleting it, that's on them. I can't find any uses of it on grep.app anyway, so I don't think anybody will have their code broken by us deleting it.
|
This PR is stale because it has been open for 30 days with no activity. |
Fixes #112319
@runtime_checkableProtocoldoesn't check if__class_getitem__exists. #112319