-
Notifications
You must be signed in to change notification settings - Fork 16
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
Added type annotations to pyi interface file. #70
Added type annotations to pyi interface file. #70
Conversation
Mypy was complaining, so I added a few more methods.
frozendict/frozendict.pyi
Outdated
self: Mapping[_KT, _VT], other: Mapping[_KT, _VT] | ||
) -> "frozendict[_KT, _VT]": ... | ||
|
||
class frozendict(Mapping[_KT, _VT], Generic[_KT, _VT]): |
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 Generic?
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 tells the type system that the class itself supports two generic types.
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.
But Generic[T1, T2]
means the class is generic and supports two parameters. This is not true, since the class is a map. There's also an example of custom dict
on typing
official docs, and it uses Mapping
, not Generic
.
frozendict/frozendict.pyi
Outdated
def copy(self: Self) -> Self: ... | ||
def __copy__(self: Self) -> Self: ... | ||
def __deepcopy__(self: Self) -> Self: ... | ||
# Omit __reduce__, its used for Pickle and we don't need the annotation in code. |
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.
? Well, also __copy__
is used for copy library, etc. Why pickle is different?
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.
Pickle needs a reference to the class as well. If we leave it out, the only people affected are those developing pickle.
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 do not understand. What's the difference with __copy__
?
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.
Anyway I think I'll merge, but have you tested it? There's a bad prototype here: https://github.com/Marco-Sulla/python-frozendict/blob/master/test/typed.py
Great job. A question: are you sure we need |
And FrozenOrderedDict only a legacy alias
removed blacklisted methods not present in the C Extension
It should be __init__, since it's a module
fix fromkeys return type
@@ -0,0 +1,54 @@ | |||
from collections.abc import Hashable |
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.
Looks like Hashable
is imported but not actually used?
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 is highly possible.
Added type annotations for this excellent library!
This includes everything for
frozendict
andFrozenOrderedDict
, but not the various helper methods.Let me know if I can do anything to help.