-
-
Notifications
You must be signed in to change notification settings - Fork 30.2k
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
gh-112319: Allow special protocol members #112340
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure which variant is better...
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.
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.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
attrs |= getattr(base, "__protocol_attrs__", set()) | ||
else: | ||
attrs |= _get_local_members(base.__dict__) | ||
return attrs | ||
|
||
|
||
def _get_protocol_attrs(cls): |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Fixes #112319
@runtime_checkable
Protocol
doesn't check if__class_getitem__
exists. #112319