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

gh-85959: (bpo-41793) Fix an inaccuracy about reflected methods in datamodel docs #22257

Merged
merged 9 commits into from
Oct 29, 2024

Conversation

wimglenn
Copy link
Contributor

@wimglenn wimglenn commented Sep 15, 2020

Concerning when the reflected operation takes precedence (docs)
Qualifying that the right operand's type must be a strict subclass avoids a counter-example when the types are actually equal

>>> class A:
...     def __add__(self, other):
...         return 1
...     def __radd__(self, other):
...         return 2
...
>>> A() + A()
1
>>> issubclass(A, A)
True

https://bugs.python.org/issue41793

…for the reflected method to take precedence avoids an edge case / counter-example when the types are actually equal.
@ammaraskar
Copy link
Member

ammaraskar commented Sep 15, 2020

Doesn't

This behavior allows subclasses to override their ancestors' operations.

basically already imply this? i.e one that's not a strict subclass would not have an "ancestor" in the class hierarchy.

@wimglenn
Copy link
Contributor Author

Guess that depends how you define ancestors (it's not in the glossary). cls.__mro__, for example, then it does include cls itself. Anyway I think it's better to be exact in the datamodel rather than imply things.

@ammaraskar
Copy link
Member

Anyway I think it's better to be exact in the datamodel rather than imply things.

Makes sense.

I don't think "strict subclass" is a well known enough term, at the very least it takes up a bit of digging on google to find a definition. Maybe it's better to be verbose and say something to the effect of, "is a subclass but not the same class (strict subclass)"

@pochmann
Copy link
Contributor

pochmann commented Sep 15, 2020

The main paragraph about these methods says:

These functions are only called if [...] and the operands are of different types. [4]

That's not fulfilled in the example. I think that takes precedence, so that the note you're talking about doesn't apply. And that the note is only about which one goes first if both could.

Also see the footnote:

[4] For operands of the same type, it is assumed that if the non-reflected method (such as __add__()) fails the operation is not supported, which is why the reflected method is not called.

(That seems to be missing a few words after "fails the operation", though, the sentence doesn't make sense to me.)

@wimglenn
Copy link
Contributor Author

@pochmann Yes, that's good point. Though it may still be an improvement for the "Note:" to be able to stand on it's own, with the strict qualifier.
The footnote you mentioned is a stumbling block, I think it may have just been an accidental omission of one word which I've added back in b1b6390

Copy link
Member

@ethanfurman ethanfurman left a comment

Choose a reason for hiding this comment

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

Remove strict.

Doc/reference/datamodel.rst Outdated Show resolved Hide resolved
Doc/reference/datamodel.rst Outdated Show resolved Hide resolved
@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.

@wimglenn
Copy link
Contributor Author

@ethanfurman I've reduced the scope of the note so that it just concerns the rationale, leaving the main paragraph to fully specify the behaviour (approximately along the lines of your suggestion). Let me know if you think that improves/simplifies things, or made it worse :)

for bot: I have made the requested changes; please review again.

@bedevere-bot
Copy link

Thanks for making the requested changes!

@ethanfurman: please review the changes made to this pull request.

Copy link
Member

@ethanfurman ethanfurman left a comment

Choose a reason for hiding this comment

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

Getting closer!

Doc/reference/datamodel.rst Show resolved Hide resolved
Doc/reference/datamodel.rst Show resolved Hide resolved
@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.

@wimglenn
Copy link
Contributor Author

wimglenn commented Sep 8, 2023

@ethanfurman I have just resolved some conflicts against main. But it has been 3 years since I created this PR, could you please clarify what changes are requested or clear the label if it looks OK? Cheers

@wimglenn
Copy link
Contributor Author

@ethanfurman I've resolved conflicts (again).

@ethanfurman
Copy link
Member

Wow, thanks for staying on this. Reviewing now.

@willingc
Copy link
Contributor

@ethanfurman I will let you determine when to merge and if to backport. Thanks!

Bravo @wimglenn for being patient and persistent

@ethanfurman ethanfurman merged commit 298e041 into python:main Oct 29, 2024
25 checks passed
@ethanfurman ethanfurman changed the title bpo-41793: Fix an inaccuracy about reflected methods in datamodel docs gh-85959: (bpo-41793) Fix an inaccuracy about reflected methods in datamodel docs Oct 29, 2024
@wimglenn wimglenn deleted the bpo41793 branch October 29, 2024 23:13
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 skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants