MNT: make _setattr_cm more conservative #17620
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
CI does not like this at all. |
|
Indeed, I thought I ran the tests before I pushed. I must have lost track of what I was doing locally... |
4111ab6 to
d0849a6
Compare
|
🤦 I edited one working tree and installed and tested a different one. |
d0849a6 to
3b4da04
Compare
anntzer
requested changes
Jun 13, 2020
lib/matplotlib/cbook/__init__.py
Outdated
| origs = {} | ||
| for attr in kwargs: | ||
| orig = getattr(obj, attr, sentinel) | ||
| # monkey patching on a new attribute, this is OK |
There was a problem hiding this comment.
perhaps you can reorder this to make the common case first, e.g.
if (attr in obj.__dict__ or orig is sentinel
or isinstance(getattr(type(obj), attr), (property, types.FunctionType)):
origs[attr] = orig
else:
raise ValueError(...)
Checking to types.FunctionType in the type is the same as checking for methods, and (as noted in the other PR, I think) won't give a wrong result if an unrelated bound method is assigned as instance var (self.foo = otherobj.method) -- at least I think this point should be fixed.
There was a problem hiding this comment.
That makes sense. I agree the method from another object is a valid concern (sorry I didn't fully register it before).
6ee399a to
756b9c3
Compare
anntzer
reviewed
Jun 14, 2020
anntzer
reviewed
Jun 14, 2020
anntzer
reviewed
Jun 14, 2020
- special case methods and properties because they are descriptors - fail on any other non-instance attribute
756b9c3 to
9e88fa5
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
PR Summary
This is a simpler version of #17561
PR Checklist