Skip to content

bpo-41397: Restore default implementation of __ne__ in Counter #21627

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

Closed

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Jul 26, 2020

return NotImplemented
return not self == other
# Restore the default implementation, override dict.__ne__
__ne__ = object.__ne__
Copy link
Contributor

Choose a reason for hiding this comment

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

Jumping mro from Counter to past dict directly to object seem like a bit of hack, one that we might regret for subclasses using super() Also, the current docstring is useful. If you really have to change something, we could just delete lines 733 and 734, leaving the instance() check for _eq_(). But that does mess-up the parallelism a bit and it looks odd so I would rather not do that either.

Copy link
Member Author

Choose a reason for hiding this comment

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

If you want to keep the docstring and make __ne__ subclass friendly, you can reimplement the logic of object.__ne__:

    def __ne__(self, other):
        'True if any counts disagree. Missing counts are treated as zero.'
        eq = self.__eq__(other)
        return NotImplemented if eq is NotImplemented else not eq

or better reuse object.__ne__:

    def __ne__(self, other):
        'True if any counts disagree. Missing counts are treated as zero.'
        return object.__ne__(self, other)

But would not be better to improve the object.__ne__ docstring so that it will be good for Counter and all other classes with customized equality comparison? BTW the current docstring of object.__ne__ is not correct because the special method can return NotImplemented.

@rhettinger
Copy link
Contributor

I recommend this be left as-is.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

I suggest to leave this be and close both the PR and the issue without merging anything.

@rhettinger rhettinger removed their assignment Dec 29, 2021
@rhettinger rhettinger closed this Dec 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants