Skip to content

Conversation

@AlexWaygood
Copy link
Member

No description provided.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

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: ...
Copy link
Member

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.

Copy link
Member Author

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? :)

Copy link
Member

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

Copy link
Member Author

@AlexWaygood AlexWaygood Feb 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Counters are sort of halfway between dicts and sets, 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.

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

filename: str
lineno: int
def __init__(self, frame: _FrameTupleT) -> None: ...
def __lt__(self, other: Frame) -> bool: ...
Copy link
Member

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.

Copy link
Member Author

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).

Copy link
Member Author

@AlexWaygood AlexWaygood Feb 14, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants