-
-
Notifications
You must be signed in to change notification settings - Fork 7.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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
I don't think that would work right e.g. for properties either. |
|
73855bf
to
136fa20
Compare
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 shadowing something 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 :)
136fa20
to
066e6ef
Compare
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. |
e6db01f
to
9abe140
Compare
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 think getattr
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. Perhaps inspect.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. |
4bf4115
to
335cd7e
Compare
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>
335cd7e
to
65e6fe4
Compare
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 (see https://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" ;) |
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