Skip to content
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

bpo-34552: Clarify built-in types comparisons #9035

Merged
merged 2 commits into from Sep 14, 2018

Conversation

Windsooon
Copy link
Contributor

@Windsooon Windsooon commented Sep 2, 2018

Some updates to ancient text about comparisons; fixes bp-34552.

@Windsooon
Copy link
Contributor Author

@gvanrossum

Copy link

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

please check the build failures and try to fix them

@Windsooon
Copy link
Contributor Author

@auvipy I fixed the failures. I think we should add skip news label in this pr

comparing a complex number with another built-in numeric type, when the objects
are of different types that cannot be compared, or in other cases where there is
no defined ordering.
Objects of different types, except different numeric types, should never compare equal.
Copy link
Member

Choose a reason for hiding this comment

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

Why did you add "should"? This is a descriptive section (explaining how builtin objects work) not a prescription for how one should implement objects in general.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right.

are of different types that cannot be compared, or in other cases where there is
no defined ordering.
Objects of different types, except different numeric types, should never compare equal.
The ``==`` defaults to :keyword:`is` if both object's :meth:`__eq__` method returns NotImplemented.
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion:

The == operator is always defined but for some object types is equivalent to :keyword:is.

Copy link
Contributor Author

@Windsooon Windsooon Sep 11, 2018

Choose a reason for hiding this comment

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

How about

The == operator is always defined but for some object types (for example, function objects) is equivalent to :keyword:is.

Copy link
Member

Choose a reason for hiding this comment

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

I'd use class objects instead of function objects -- the intro to this section (L15) mentions those explicitly.

FWIW reading that section makes me think we should replace compared on L23 with compared for equality.

The ``<=``, ``>`` and ``>=`` operators aren't always defined, they will raise a
:exc:`TypeError` exception when comparing a complex number with another built-in
numeric type, when the objects are of different types that cannot be compared,
or in other cases where there is no defined ordering.
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion:

The <=, > and >= operators are only defined where they make sense; for example, they raise a
:exc:TypeError exception when one of the arguments is a complex number.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we talk about != operator here? I'm not sure is != always defined just like ==.

Copy link
Member

Choose a reason for hiding this comment

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

In new enough 3.x versions, if you define __eq__ but leave __ne__ undefined, != falls back to not ==. I don't think it's worth mentioning, since this is not the language reference, just a brief summary of typical behavior of common objects.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@gvanrossum
Copy link
Member

Can you add a news blurb? That will clear the bedevere/news test failure.

Also please don't amend your local commit and git push -f -- instead, create a new local commit and git push. This is easier for the reviewers. Your stack of local commits will be squashed when we merge the diff.

@Windsooon
Copy link
Contributor Author

Can you add a news blurb? That will clear the bedevere/news test failure.

Also please don't amend your local commit and git push -f -- instead, create a new local commit and git push. This is easier for the reviewers. Your stack of local commits will be squashed when we merge the diff.

Thank you for point it out.

@gvanrossum
Copy link
Member

@Windsooon DO you have time to push a new version reflecting the latest discussion?

@Windsooon
Copy link
Contributor Author

@gvanrossum I do have time, I added the blurb, but I'm not sure what should I do next?

@gvanrossum
Copy link
Member

Ah, sorry. I'm not sure what happened -- maybe you amended your commit, maybe I didn't re-check the latest version. I am now happy and will let the bot do the rest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants