bpo-8722: Document __getattr__ behavior with AttributeError in property #4754
Conversation
terryjreedy
left a comment
terryjreedy
left a comment
There was a problem hiding this comment.
In title, change 'DOC:' to document.
Suggested message for commit and blurb:
When a property get method raises AttributeError, getattr is called if it exists. This is the same as when getattribute fails to find an attribute.
Doc/reference/datamodel.rst
Outdated
| an :exc:`AttributeError` because *name* is not an instance attribute, or | ||
| when it is not found in the class tree for ``self``. This method should | ||
| return the (computed) attribute value or raise an :exc:`AttributeError` | ||
| exception. |
There was a problem hiding this comment.
You changed my suggested revision. I am guessing because it was slightly off, in that when a property raises, getattribute does not. However, I prefer to change 'usual places' because a property is not a place. I also prefer referring to properties second. How about:
Called when normal attribute lookup fails with an AttributeError. Either name is neither an instance attribute nor an attribute in the class tree for self, and meth:__getattribute__ raises an :exc:AttributeError; or :meth:__get__ of a name property raises :exc:AttributeError. This method should either return ... or raise... .
Any further revision?
There was a problem hiding this comment.
I left out the backticks needed to quote double underscores so that they are not interpreted as bold markup.
There was a problem hiding this comment.
I didn't mean to take away any of the intention with your original suggested revision. From other tickets I've looked at, I've learned that there's a balance between maintaining some of the existing conversational feel to the docs (especially if it's existed for a long time) when modifying the docs, so I was trying to incorporate that. I was trying not to step on toes (and I stepped on yours :-) )
I had moved properties first because when looking at the attribute access order docs, it said that a descriptor method might override the default behavior, so I tried to indicate an order of application, but I don't think it matters. Actually, maybe instead of 'normal attribute lookup', we should use 'default attribute access', like in https://docs.python.org/3/howto/descriptor.html?highlight=descriptors#definition-and-introduction?
For revisions, the 'Either.. neither...' line somehow doesn't sound quite right. Maybe it should all be after a semi-colon or dash?
Called when the default attribute access fails with an AttributeError - either meth:getattribute raises an :exc:AttributeError because name is not an instance attribute or an attribute in the class tree for self; or :meth:get of a name property raises :exc:AttributeError. This method should either return ... or raise... .
There was a problem hiding this comment.
@terryjreedy Do you think a test should be added to test_descr.py for this scenario (ie, combining a property with __getattr__)?
|
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 |
csabella
changed the title
|
@terryjreedy I made some changes to reflect the conversation we had on the last comment. You had posed several questions on the bug tracker. Was there anything you needed from me on that? |
|
Please add a blurb, which will trigger re-testing. We can revise if we expand to set/delattr. |
|
@terryjreedy Added a blurb. |
|
I gather that markup is now allows in blurbs. Does it result in links from the html log to the docs? If Nick does not respond, I will ask for another reviewer on core-mentorship. After 8 years, this should be finished ;-). |
|
@terryjreedy In the blurb docs, it says:
The 3.7 changelog does link back to the docs. https://docs.python.org/3.7/whatsnew/changelog.html#changelog |
ncoghlan
left a comment
ncoghlan
left a comment
There was a problem hiding this comment.
I'm away from my computer this weekend, and the one behaviour I'd want to double check is whether overriding __getattribute__ without calling up to super().__getattribute__ also skips the __getattr__ fall back.
The docs could be made independent of that by deleting the phrase ":meth:__getattribute__ raises an :exc:AttributeError because" and just leaving the explanation of how that may fail.
|
After nearly 8 years, a couple of days will not hurt ;-). |
|
Huh, turns out I was wrong and the proposed docs are right (the So I'll go ahead and merge this. Thanks @csabella! :) |
|
GH-5542 is a backport of this pull request to the 3.6 branch. |
…ty (pythonGH-4754) When `__getattr__` is implemented, attribute lookup will always fall back to that, even if the initial failure comes from `__getattribute__` or a descriptor's `__get__` method (including property methods). (cherry picked from commit d1f3181) Co-authored-by: Cheryl Sabella <[email protected]>
|
GH-5543 is a backport of this pull request to the 3.7 branch. |
…ty (pythonGH-4754) When `__getattr__` is implemented, attribute lookup will always fall back to that, even if the initial failure comes from `__getattribute__` or a descriptor's `__get__` method (including property methods). (cherry picked from commit d1f3181) Co-authored-by: Cheryl Sabella <[email protected]>
6 participants
When a property get method raises an
AttributeError,__getattr__is called if it exists. This is the same as when__getattribute__fails to find an attribute.https://bugs.python.org/issue8722