Skip to content

Improve collections.Counter stub #7464

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

Merged
merged 7 commits into from
Mar 9, 2022
Merged

Improve collections.Counter stub #7464

merged 7 commits into from
Mar 9, 2022

Conversation

AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Mar 8, 2022

TwoThree changes:

  1. RemoveChange the bad overload which the comment states is bad, and use a # type: ignore instead.
  2. Improve Counter arithmetic dunders. Counter is halfway between a set and a dict, and the arithmetic dunders come from the set-like side of its personality. I think it makes sense to harmonise Counter's __sub__, __and__, __or__ and __add__ methods with the equivalent ones on typing.AbstractSet.
  3. Use Counter[Any] instead of Counter[object] in various Counter dunders, due to dict invariance.

@AlexWaygood AlexWaygood marked this pull request as draft March 8, 2022 21:21
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@AlexWaygood AlexWaygood marked this pull request as ready for review March 8, 2022 21:37
def __or__(self, other: Counter[_T]) -> Counter[_T]: ... # type: ignore[override]
def __add__(self, other: Counter[_S]) -> Counter[_T | _S]: ...
def __sub__(self, other: Counter[Any]) -> Counter[_T]: ...
def __and__(self, other: Counter[Any]) -> Counter[_T]: ...
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we should change to Any, unless users complain. With Any, you won't get an error for counter_of_ints - counter_of_strings, which is bad IMO. With _T, you will get an error for counter_of_strings - counter_of_strings_or_nones, which apparently doesn't come up in practice because nobody has complained about it.

Copy link
Member Author

Choose a reason for hiding this comment

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

The same argument surely applies for Abstract Set.__sub__:

def __sub__(self, other: AbstractSet[Any]) -> AbstractSet[_T_co]: ...

Are we saying that in an ideal world, that also wouldn't be AbstractSet[Any], but it would probably be too disruptive to change it now?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Changing AbstractSet would be more disruptive, but the argument doesn't apply to AbstractSet as much as it does to Counters:

  • It's common to have sets that contain Nones and subtract between them, which I think is the most common use for subtracting sets of different types. I use sets containing Nones all the time.
  • A counter that contains Nones would be something like {None: 2, "foo": 1, "bar": 6}, which seems like it could useful in some case but not very often. I don't remember ever using a counter like this, for example.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm still sorta unconvinced. I'll revert for now, and maybe open an issue to discuss in more depth later :)

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

1 similar comment
@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2022

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

@AlexWaygood AlexWaygood requested a review from Akuli March 9, 2022 09:53
@Akuli Akuli merged commit 4e87b90 into python:master Mar 9, 2022
@AlexWaygood AlexWaygood deleted the counter branch March 9, 2022 10:24
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