-
-
Notifications
You must be signed in to change notification settings - Fork 2k
[typing] Add missing _from_iterable classmethod to Set ABCs
#15331
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Emmanuel Ferdman <emmanuelferdman@gmail.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Hey @srittau could you please review this small PR when you get the chance? thank you! |
stdlib/typing.pyi
Outdated
| class ItemsView(MappingView, AbstractSet[tuple[_KT_co, _VT_co]], Generic[_KT_co, _VT_co]): | ||
| def __init__(self, mapping: SupportsGetItemViewable[_KT_co, _VT_co]) -> None: ... # undocumented | ||
| @classmethod | ||
| def _from_iterable(cls, it: Iterable[Any]) -> set[Any]: ... |
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.
Why not def [S](cls, it: Iterable[S]) -> set[S] ?
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.
Good call, updated to use Iterable[_S] -> set[_S]. Used the existing _S TypeVar since PEP 695 syntax isn't used elsewhere in typeshed.
stdlib/typing.pyi
Outdated
| def _hash(self) -> int: ... | ||
| # Mixin methods | ||
| @classmethod | ||
| def _from_iterable(cls, it: Iterable[Any]) -> AbstractSet[Any]: ... |
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 should return Self, not any AbstractSet, since the implementation is return cls(it) and other methods on MutableSet that rely on this are annotated to return Self.
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.
Updated to AbstractSet[_S] for type preservation, but can't use Self here - KeysView and ItemsView override this to return set, not Self. Using Self on the base would make those overrides invalid. Also MutableSet.__ior__/__iand__ etc. don't actually call _from_iterable, they mutate in-place directly.
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.
Actually, the type we want here is more like def [S](cls, Iterable[S]) -> Self & AbstractSet[S], which is not expressible at the moment.
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.
So -> AbstractSet[S] seems like the best approximation atm.
Signed-off-by: Emmanuel Ferdman <emmanuelferdman@gmail.com>
|
The failing test is unrelated to this PR - Python 3.9 support is EOLd I guess. |
|
Diff from mypy_primer, showing the effect of this PR on open source code: discord.py (https://github.com/Rapptz/discord.py)
- ...typeshed_to_test/stdlib/typing.pyi:1049: note: "update" of "TypedDict" defined here
+ ...typeshed_to_test/stdlib/typing.pyi:1055: note: "update" of "TypedDict" defined here
|
|
Btw. in the original issue I posted a comment that this is probably a bad idea at the moment -- sorry I didn't mention it here earlier, I didn't realize this was a different account than the OP of #15298 |
PR Summary
This PR adds
_from_iterabletoAbstractSet(returningAbstractSet[_S]to preserve element types while allowing subclasses that return set), and toKeysView/ItemsViewwhich override it to returnset[_S].MutableSetinherits fromAbstractSetso no separate definition is needed.Fixes #15298.