Repository navigation
MNT: make _setattr_cm more conservative - #17620
anntzer merged 1 commit into
Conversation
Sorry, something went wrong.
|
CI does not like this at all. |
Sorry, something went wrong.
|
Indeed, I thought I ran the tests before I pushed. I must have lost track of what I was doing locally... |
Sorry, something went wrong.
|
🤦 I edited one working tree and installed and tested a different one. |
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
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).
Sorry, something went wrong.
anntzer
left a comment
There was a problem hiding this comment.
just style things.
Sorry, something went wrong.
- special case methods and properties because they are descriptors - fail on any other non-instance attribute
PR Summary
This is a simpler version of #17561
PR Checklist