Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add support for cached_properties to slotted attrs classes. #1200
Add support for cached_properties to slotted attrs classes. #1200
Changes from 10 commits
3e31d8c
6bbc0a6
6737a28
f8d2739
6860faf
3e7ad4f
74205c5
804d33c
80e8a6b
b0d87e3
07741dd
bac209e
41bed43
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have to do this check here? Can this be checked at class construction time and passed as a parameter to
_make_cached_property_getattr
? Might make the__class__
global unnecessary too.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've moved the check out to construction, and passed in the
original_getattr
to avoid the class lookup.__class__
is still needed in the closure for thesuper()
call to work.I've removed
__attrs_original_getattr__
on the class, as a separate MR, because it needed some other changes for it goes through the__class__
closure reference updating, which has it's own trade-off.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
There was a problem hiding this comment.
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 can do this at construction, because a
__getattr__
might exist only on a different super-class of a sub-class. We could optimise for the case that it exist with atry/except
on the attribute access instead of theif
, since I guess theAttributeError
caused by a non-existant__getattr__
isn't the most critical thing to be fast.For reference in case it influences the decision:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying to understand the case we're guarding against here.
The situation you're describing, as I understand it, is this class sandwich:
Let me know if any of my reasoning is faulty.
The
__getattr__
onC
needs to callsuper()
, but we can tell at class construction time.The
__getattr__
onA
doesn't need to callsuper()
, andsuper()
wouldn't go toB
anyway, sinceB
isn't a superclass of A.So we should be able to do this check at class construction time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, should have been clearer, I was thinking of something along the lines of:
the
__getattr__
onA
needs to callsuper
so that it passes to the__getattr__
onMixin
, but can't know about it's existence at construction time.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see. I suspected multiple inheritance might be the thing complicating our life here ;)