Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
MNT: handle generic descriptors in _setattr_cm#17561
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
I don't think that would work right e.g. for properties either. |
jklymak commentedJun 3, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
|
I added a test for a property as well, it seems to work. Noticed this while I was working on#17515 (where I ended up just doing it all "by hand") and this will be relevant for#17560 . I do see how this is still going to go sideways for class attributes (as we end up with an equal value at the instance level shadowingsomething is the class hierarchy), but I think that this is still a net improvement. |
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 guess it's already an improvement :)
Uh oh!
There was an error while loading.Please reload this page.
Added comments with links to both the tests and the implementation. I had tweaked the implementation a bit to give a better path to support un-masking class level attributes but in the process of writing out how it could be implemented I saw how to implement it. If people are unsure, I'm happy to drop the second commit. |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
lib/matplotlib/cbook/__init__.py Outdated
if attr in obj.__dict__: | ||
return True | ||
if isinstance(getattr(type(obj), attr), property): |
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.
Perhaps safer would be:
if attr in type(obj).__dict__ and hasattr( type(obj).__dict__[attr], '__set__'):
which covers all descriptors
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 thinkgetattr
is better here so we see the super classes, but checking__set__
seems better 👍
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.
getattr
will invoke some descriptors still. Perhapsinspect.getattrstatic
?
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.
🤦 The rabbit hole gets deeper!
Well, this started as a special case for methods and turned into the full general case for descriptors and class level attributes... |
This is also now a bigger change than I think we should backport. |
Pushed to 3.4, can be moved back but I would rather focus on other PRs for 3.3. |
If you want to get something in for 3.3, can we instead just error out for anything that's not in the instance |
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
I rebased this, but am going to orphan it. I think the next step here is to write out a couple more pathological test cases involving custom descriptors. Labeled as "good first issue" because there is not API design ("it goes back to as it was" is the API) but medium difficulty as it requires and understanding of the Python descriptor protocol (seehttps://docs.python.org/3/howto/descriptor.html ) which is not basic Python. |
(I think we should just support instance attributes and a small, explicitly listed subset of descriptors (each of which should be motivated by an actual use case), as writing a general utility seems to be way more complex than it is really worth). |
Fair, I'm also open to "close this as a bad idea" ;) |
timhoffm commentedApr 15, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I agree with@anntzer. Let's do all the things we need but not more (=motivated by an actual use case). |
Given the feedback, I'm going to close this. If we end up needing something more sophisticated we will either remember and revive this or implement something more narrowly tailored. |
PR Summary
If we are monkey-patching methods then we need to be sure that when
we restore the original attribute we need to make sure we don't
stick the method instance in place (which creates a circular
reference).
PR Checklist