-
Notifications
You must be signed in to change notification settings - Fork 206
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
Add type annotations to sortedcontainers #107
Conversation
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 looks pretty good.
I’m a Python typing newbie. Three questions: How do I run it myself? How do I test it? Can I integrate the testing with tox?
Well, I'm not a typing master myself, but basically, you'll need a typechecker set-up, for instance Then you can run the typechecker first of all on the codebase itself (e.g. AFAIK, there's no way to test the typing is good (that's like testing a program is without bugs), but maybe you could try to have test cases with erroneous typing to see it is detected (such as appending an |
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 I'm using this project and have written stubs for other projects (asyncpg, sqlalchemy, gino, etc.) I figured I could help out here. I added a bunch of comments for sorteddict
that also apply for the other files. Let me know if something I commented on doesn't make sense.
@bryanforbes: thanks for your review ! I'll add the required changes soon. |
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 updates so far look great, but I found a few more issues :D
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 copied the stub files into a project I'm using and with the changes I've outlined in this round of reviews, I get no errors from mypy. I'll approve once the feedback is addressed.
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 great!
Thanks for doing this, @althonos. You beat me to it :). |
@bryanforbes : thanks for reviewing my code ! I let so much slip through it's mostly thanks to you this PR became high-standard enough 😉 |
@grantjenks This is ready to merge |
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'm learning a lot looking through these changes. The tone of my questions are curiosity/learning, not challenges. There's 7 questions so far :)
def __init__(self, __map: Mapping[_KT, _VT], **kwargs: _VT) -> None: ... | ||
@overload | ||
def __init__( | ||
self, __iterable: Iterable[Tuple[_KT, _VT]], **kwargs: _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.
Does Tuple[_KT, _VT] here mean strictly a tuple? The code will work for any two-element sequence.
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's a two-value tuple with the generic type _KT
as its first element and _VT
as its second element. I recommend reading the section on generics in the mypy docs for a more in-depth understanding of the generic types.
def __iter__(self) -> Iterator[_KT]: ... | ||
def __reversed__(self) -> Iterator[_KT]: ... | ||
def __setitem__(self, key: _KT, value: _VT) -> None: ... | ||
def _setitem(self, key: _KT, value: _VT) -> None: ... |
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 document protected/private methods? These aren't part of the API.
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 reason I didn't call this out in my review is that sometimes it's useful to have these methods in the stubs so they can be used in subclasses. If these are not intended to be used in subclasses, they should probably be removed.
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 you call a private method from inside a public method the type can then be derived there from the type hints. For instance, PyCharm warns on the last line of this code, because it could derive the return type of the public method pub
from the private method __private
:
class Klass:
def __private(self) -> int:
return 0
def pub(self):
return self.__private()
def pub2(self, a: str):
pass
O = Klass()
O.pub2(O.pub())
To make sure that the type hints actually are correct within sortedcontainers, it's probably a good idea to commit fully to static type checking.
def __reversed__(self) -> Iterator[_KT]: ... | ||
def __setitem__(self, key: _KT, value: _VT) -> None: ... | ||
def _setitem(self, key: _KT, value: _VT) -> None: ... | ||
def copy(self: _SD) -> _SD: ... |
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 the annotation for self
here and not for the other methods?
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.
copy
returns the same type as the instance, so we need to mark this in such a way that the type returned from subclasses will be properly derived. Using a generic self is how we do that. If a method doesn't need to return that information, then annotating self
isn't needed.
sortedcontainers/sorteddict.pyi
Outdated
def __copy__(self: _SD) -> _SD: ... | ||
@classmethod | ||
@overload | ||
def fromkeys(cls, seq: Iterable[_T]) -> SortedDict[_T, None]: ... |
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 be Iterable[_KT]
? I think the elements must be 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.
I don't think it should be _KT
because it would derive _KT
from the class's declaration. We should probably declare another TypeVar
above that is bound to Hashable
.
def pop(self, key: _KT) -> _VT: ... | ||
@overload | ||
def pop(self, key: _KT, default: _T = ...) -> Union[_VT, _T]: ... | ||
def popitem(self, index: int = ...) -> Tuple[_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.
The extra spaces in int = ...
look a bit odd to me. Is there a style guide for these type files?
Also, is int
the best choice? Is there no Integral
or size_t
equivalent?
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 typeshed style guide is what most people use.
Yes, int
is the right choice. It's what list.popitem()
uses in typeshed's typing.
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 what it's worth, pep8 also suggests this usage of spaces:
When combining an argument annotation with a default value, however, do use spaces around the = sign
sortedcontainers/sorteddict.pyi
Outdated
def pop(self, key: _KT, default: _T = ...) -> Union[_VT, _T]: ... | ||
def popitem(self, index: int = ...) -> Tuple[_KT, _VT]: ... | ||
def peekitem(self, index: int = ...) -> Tuple[_KT, _VT]: ... | ||
def setdefault(self, key: _KT, default: Optional[_VT] = ...) -> _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 specify default
here and not use an overload as with fromkeys
?
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.
With fromkeys()
, the return value is dependent upon whether value
is specified or not. I went and checked the implementation of setdefault()
and I can see that this typing is wrong and it should actually be:
def setdefault(self, key: _KT, default: Optional[_VT] = ...) -> Optional[_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.
Actually, couldn't this be overloaded to:
@overload
def setdefault(self, key: _KT) -> Optional[_VT]: ...
@overload
def setdefault(self, key: _KT, default: _VT = ...) -> _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.
Hmmm... the stdlib has the following:
def setdefault(self, k: _KT, default: Optional[_VT] = ...) -> _VT: ...
However, I can do the following:
d = SortedDict()
v = d.setdefault('a') # v is None
v = d.setdefault('a', 1) # v is still None
I'm going to ask in the mypy gitter room, but returning _VT
seems wrong based on the code above. I'll let you know what I find out.
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.
@bryanforbes : my bad, this would be a thing for dict.get
, but not for dict.setdefault
, you're right.
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.
@althonos From my conversation in the Gitter room, we should leave this the same as what the stdlib has and I'm going to file an issue to discuss it on typeshed. The Optional[_VT]
return type might produce a lot of false-positives.
sortedcontainers/sorteddict.pyi
Outdated
def __getitem__(self, index: slice) -> List[_KT_co]: ... | ||
def __delitem__(self, index: Union[int, slice]) -> None: ... | ||
|
||
class SortedItemsView( # type: ignore |
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.
What does type: ignore
mean?
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.
Currently, there's an issue with the definitions for ItemsView.__iter__()
and Sequence.__iter__()
in typeshed
. The type: ignore
comment tells mypy to ignore the error between those two signatures (until the linked issue gets fixed).
Great questions, @grantjenks, and I didn't find them as challenges. Hopefully my answers were helpful and not condescending. |
I looked around a bit and searched typing.py but could not find the typing hints for OrderedDict and the like. Are there pyi files for the built-in types? How do I compare what’s here with the standard library? |
@grantjenks: they can be found in the python/typeshed repository: for instance, |
Any updates on this? I'm using sortedcontainers in a typed project and it would be great to have the type hints available. |
I'm making progress on reviewing the changes but it's a large addition and a new kind of feature for me. |
thanks to everyone involved with this work (starting with |
I would love to see this merged too. Great work. |
I apologize for the delay. I have recently learned more about mypy and type annotations. I may be able to get something rolled out by the end of the month. Until then, I'm open to the type annotations being added to typeshed: https://github.com/python/typeshed |
Sorry for the great delay in this pull request. I'm keeping it in my Inbox now and looking for time to review the changes and merge. |
Just tried using this and got the following error:
Installed with Am I doing something wrong or is this an issue? |
@nathanielobrown : kinda expected, since we are only adding stub files, and not changing anything in the code, including the It probably works with s: "SortedList[int]" = SortedList() |
I don't know the answer to 1, but for 2, anecdotally: in PyTorch we're still using the old |
@grantjenks Perhaps mypyc could be a plausible alternative? https://github.com/mypyc/mypyc Also: take your time. Open source burnout is real, and it's your free time. |
Ditto on this; please don't take my earlier comments as a demand, just curious about the state of things |
Please don't feel any pressure from my side, I know reviewing large PRs takes time and dedication. Now trying to answer the questions:
Basically your
Indeed as @samestep mentioned, for Python 3.6 compatibility you should still use |
@althonos I don't believe this is true; quoting PEP 484:
As another example, if I remember correctly, typing something as (I'm not at my computer right now, but once I am, I'll double-check this and edit my comment accordingly.) |
@samestep : You have activated my trap card! To your PEP 484 I raise you PEP 585! Joke aside, from Python 3.9 onward, To fix that in the Python code of From the perspective of EDIT: Oh and about the section you quote, it's only relevant for the annotations within Python sources. That PEP explicitly states the following afterwards:
So the version of your stub only matters for version of |
Ah, you are correct; I was very confused when reading the first part of your comment because I tried this in Python 3.6 just now and it didn't work:
But I had forgotten that this PR only adds the type annotations in |
Hi, any chance to advance on this? |
Sorry, no. It still hasn't made it to the top of my list. If those interested want to publish a sortedcontainers-types package with the pyi files, then I'm okay with that. |
self, | ||
start: Optional[int] = ..., | ||
stop: Optional[int] = ..., | ||
reverse=bool, |
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 think this might be a typo?
reverse=bool, | |
reverse: bool = ..., |
Edit: and similarly in sorteddict.pyi
and sortedset.pyi
def irange( | ||
self, | ||
minimum: Optional[_T] = ..., | ||
maximum: Optional[_T] = ..., | ||
inclusive: Tuple[bool, bool] = ..., | ||
reverse: bool = ..., | ||
): ... | ||
def irange_key( | ||
self, | ||
min_key: Optional[Any] = ..., | ||
max_key: Optional[Any] = ..., | ||
inclusive: Tuple[bool, bool] = ..., | ||
reserve: bool = ..., | ||
): ... |
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 think these both return Iterator[_T]
? That aside, I think the irange
stub is redunant here---it should inherit from the stub for SortedList.irange?
I've mentioned my changes on the PR these stubs come from: grantjenks/python-sortedcontainers#107
Guys, this PR is soon reaching its fifth birthday ... that should have been enough time for a review. Just get it merged! |
@ml31415 Sorry, no. It still hasn't made it to the top of my list. If those interested want to publish a sortedcontainers-types package with the pyi files, then I'm okay with that. |
Look @grantjenks this is an open source package, no one needs your goodwill for forking this package. People here try to improve this package in order to reduce the burden of maintenance for all users and maintainers alike, i.e. not create another fork. Everyone tries to avoid forking! But in order for this collaboration model to work, well written PRs also need to be accepted at some point. And right now this would need to be done by either you, or someone you'd be ready to trust to help you with the maintenance of this project, maybe a former collaborator? Or one of the friendly people here, that wrote PRs and already showed interest in improving this package? It's never a good thing if the future of a package depends only on a single person. |
grantjenks/python-sortedcontainers#107 (review) Co-authored-by: David Robertson <david.m.robertson1@gmail.com>
grantjenks/python-sortedcontainers#107 (review) Co-authored-by: David Robertson <david.m.robertson1@gmail.com>
In the original PR there were different opinions on this, but now that the types are separate from the code, it won't help the development of sortedcontainers itself to type the internal methods, so I think it's best to not publicise them to library users. See: grantjenks/python-sortedcontainers#107 (comment)
It looks to me like a good way forward here is to publish a stand-alone type stubs package, and let Grant continue to maintain the implementation package in the way he's most comfortable with. So I've created a separate repo with just the type stubs from this PR, tested the stubs, made some tweaks and published a package on pypi. If you |
Thanks @h4l ! |
Closing in favor of https://pypi.org/project/sortedcontainers-stubs/ If you'd like to add something to the README to mention these are available, PR welcome. |
The README now mentions and links to the `sortedcontainers-stubs` package, as discussed in grantjenks#107.
The README now mentions and links to the `sortedcontainers-stubs` package, as discussed in grantjenks#107.
Hi @grantjenks,
this PR adds type annotations to the
sortedcontainers
package. They should be consistent with theirlist
,set
anddict
annotations from thetypeshed
annotations. All collections are invariant generics.Here are the current quirks:
__new__
annotations are not supported, somypy
does not understand that creating aSortedList
with a key argument returns aSortedKeyList
instance (this is amypy
bug, see overloading __new__ python/mypy#3307)this has been fixed inSortedItemsView
raises an error because of supposedly incompatible base classes (this is amypy
bug, see Definition of "__iter__" in "ItemsView" is incompatible with definition "Sequence" python/mypy#5973)mypy
typing
exposesProtocol
s, it is not really possible to set the return type ofkey
functions. This means that the acceptable type for a key isCallable[[T], Any]
. In particular, this also means thatbisect_key_left
andbisect_key_right
acceptAny
. The other way around would be to makeSortedList
a generic over bothT
andK
whereK
is the key type, but that would possibly be a hindrance to end users.SortedKeyList.__contains__(value)
should accept anything (as anySequence
), but since the value is passed toself._key
, the value should be of typeT
for the code never to fail. The choices are: either only acceptT
, typecheck the value in the code, or return False on anyTypeError
raised by the key. None of them are really satisfactory IMO.SortedDict.setdefault
has the same signature thandict.setdefault
from the standard library, although in theory the signature should have an overload if not default is given.