GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||
GeezerVison is showing github.com — MNT: handle generic descriptors in _setattr_cm by tacaswell · Pull Request #17561 · matplotlib/matplotlib Conversation
Member
PR SummaryIf we are monkey-patching methods then we need to be sure that when PR Checklist
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Contributor
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
|
There was an error while loading. Please reload this page.
|
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I guess it's already an improvement :)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
e6db01f to
9abe140
Compare
There was an error while loading. Please reload this page.
| if attr in obj.__dict__: | ||
| return True | ||
|
|
||
| if isinstance(getattr(type(obj), attr), property): |
There was a problem 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
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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 👍
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
🤦 The rabbit hole gets deeper!
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Well, this started as a special case for methods and turned into the full general case for descriptors and class level attributes... |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
This is also now a bigger change than I think we should backport. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
4bf4115 to
335cd7e
Compare
|
Pushed to 3.4, can be moved back but I would rather focus on other PRs for 3.3. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
If you want to get something in for 3.3, can we instead just error out for anything that's not in the instance |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Co-authored-by: Elliott Sales de Andrade <[email protected]>
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
(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). |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Fair, I'm also open to "close this as a bad idea" ;) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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). |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.