-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
stdlib: Add several missing comparison methods #7202
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
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@@ -266,6 +270,10 @@ class Counter(dict[_T, int], Generic[_T]): | |||
def __ior__(self: Self, other: Counter[_T]) -> Self: ... # type: ignore[override] | |||
if sys.version_info >= (3, 10): | |||
def total(self) -> int: ... | |||
def __le__(self, other: Counter[object]) -> 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 we should use Counter[Any] here. It doesn't work with all Counters, only with Counters of a type that's comparable to our type. That's something we can't express in the current type system, so we should use 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.
>>> from collections import Counter
>>> Counter('abcde') < Counter((1, 2, 3))
False
What am I missing? :)
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.
Ah right, the type parameter is the keys, but it compares the values, which are always ints.
This does make for some fun behavior:
In [16]: Counter((1, 2, 3)) < Counter('abcde')
Out[16]: False
In [17]: Counter('abcde') < Counter((1, 2, 3))
Out[17]: False
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.
Counter
s are sort of halfway between dict
s and set
s, and the comparison methods come from the set
-like side of their personality, so it does sort of make sense if you squint at it from the right angle.
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
@@ -41,6 +41,15 @@ class Frame: | |||
filename: str | |||
lineno: int | |||
def __init__(self, frame: _FrameTupleT) -> None: ... | |||
def __lt__(self, other: Frame) -> 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.
Confirmed that __lt__
for some reason does not have the NotImplemented trick.
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 because it's functools.total_ordering
that uses the NotImplemented
trick, and __lt__
is the only method that's actually defined on the class (all the other methods are autogenerated using @functools.total_ordering
).
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 recent change is why I used the sys.version_info
guards, FWIW: python/cpython#30818
No description provided.