| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
I don't think that would work right e.g. for properties either. |
Sorry, something went wrong.
|
This closes #17542 ? EDIT: sorry, confused the PRs... |
Sorry, something went wrong.
|
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.
Sorry, something went wrong.
There was a problem hiding this comment.
I guess it's already an improvement :)
Sorry, something went wrong.
|
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.
| if attr in obj.__dict__: | ||
| return True | ||
|
|
||
| if isinstance(getattr(type(obj), attr), property): |
There was a problem hiding this comment.
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 a problem hiding this comment.
I think getattr is better here so we see the super classes, but checking __set__ seems better 👍
Sorry, something went wrong.
There was a problem hiding this comment.
getattr will invoke some descriptors still. Perhaps inspect.getattrstatic?
Sorry, something went wrong.
There was a problem hiding this comment.
🤦 The rabbit hole gets deeper!
Sorry, something went wrong.
|
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.
|
This is also now a bigger change than I think we should backport. |
Sorry, something went wrong.
|
Pushed to 3.4, can be moved back but I would rather focus on other PRs for 3.3. |
Sorry, something went wrong.
|
If you want to get something in for 3.3, can we instead just error out for anything that's not in the instance __dict__ for now (and perhaps special case methods too), and add in features in an as-needed fashion? I'm not sure setattr_cm makes sense for arbitrary properties (or worse, arbitrary descriptors) anyways... |
Sorry, something went wrong.
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 (see https://docs.python.org/3/howto/descriptor.html ) which is not basic Python. |
Sorry, something went wrong.
|
(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.
|
Fair, I'm also open to "close this as a bad idea" ;) |
Sorry, something went wrong.
|
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.
|
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.
| Back | FazBrowse Home | New Git URL |
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