Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upRefactoring to start using getattr_static #832
Conversation
Avoids having to use AttrCleaner in many contexts
|
Good stuff! Using AttrCleaner in one more spot and fixing some small things are all that are left. |
| else: | ||
|
|
||
| def get_attr_safe(obj, name): | ||
| """side effect and AttrCleaner free getattr (calls getattr_static).""" |
This comment has been minimized.
This comment has been minimized.
thomasballinger
Aug 1, 2020
Member
"AttrCleaner free" is confusing to me because nobody knows what AttrCleaner does, I'd move that part to a comment.
| with inspection.AttrCleaner(value): | ||
| if inspection.is_callable(value): | ||
| word += "(" | ||
| # TODO: do we need this done within an AttrCleaner? |
This comment has been minimized.
This comment has been minimized.
thomasballinger
Aug 1, 2020
Member
It doesn't look like it: I tried running callable on an instance of a class with a broken __getattr__ and a broken __getattribute__, and with or without a __call__() method, in Python 2 and 3, these didn't crash or run their side effects.
https://github.com/python/cpython/blob/cadda52d974937069eeebea1cca4229e2bd400df/Objects/object.c#L1434 also suggests this wouldn't be intercepted by either of these.
|
|
||
| # strips leading dot |
This comment has been minimized.
This comment has been minimized.
thomasballinger
Aug 1, 2020
Member
I think the intention of the spacing in front of this comment was to line it up with the m[1:] below it
| @@ -356,16 +356,15 @@ def attr_matches(self, text, namespace): | |||
| obj = safe_eval(expr, namespace) | |||
| except EvaluationError: | |||
| return [] | |||
| with inspection.AttrCleaner(obj): | |||
| matches = self.attr_lookup(obj, expr, attr) | |||
| matches = self.attr_lookup(obj, expr, attr) | |||
This comment has been minimized.
This comment has been minimized.
thomasballinger
Aug 1, 2020
Member
The comment about about monkeypatching to prevent side-effects (__getattr__/__getattribute__) is out of date now that we no longer use AttrCleaner here. Let's remove the comment. This is fine because 1) we don't support 2.6 anymore, and 2) we don't call callable anywhere in here anyway.
Also, if we're going to remove AttrCleaner then attr_lookup() and this method should be combined; the whole point of separating them was so that attr_lookup() could be run in the AttrCleaner context manager.
| if not py3: | ||
|
|
||
| def get_attr_safe(obj, name): | ||
| """side effect free getattr""" |
This comment has been minimized.
This comment has been minimized.
thomasballinger
Aug 1, 2020
Member
While we're in this code, how about we rename get_attr_safe to getattr_safe and has_attr_safe to hasattr_safe; getattr and hasattr are Python builtins, and our semantics are trying to match those but be safer.
| return matches | ||
|
|
||
| def attr_lookup(self, obj, expr, attr): | ||
| """Second half of original attr_matches method factored out so it can | ||
| be wrapped in a safe try/finally block in case anything bad happens to | ||
| restore the original __getattribute__ method.""" | ||
| words = self.list_attributes(obj) | ||
| if hasattr(obj, "__class__"): | ||
| if inspection.has_attr_safe(obj, "__class__"): | ||
| words.append("__class__") | ||
| words = words + rlcompleter.get_class_members(obj.__class__) |
This comment has been minimized.
This comment has been minimized.
thomasballinger
Aug 1, 2020
Member
Running
>>> class BrokenGetAttr(object):
... def __getattribute__(self, name):
... print('running custom getattribute on', name)
... import traceback
... traceback.print_stack()
...
>>> b = BrokenGetAttr()
>>> b.
shows me four times when a custom __getattribute__ is invoked: this obj.__class__, the one the line below, and twice from the dir() on 390.
For the first two we can grab __class__ once with static_getattr, but I'm not sure about the dir call; I guess it still needs AttrCleaner? It's just __dict__ and __class__ that dir seems to check for. I didn't find this very helpful but the implementation of dir() we're using is usually this one: https://github.com/python/cpython/blob/cadda52d974937069eeebea1cca4229e2bd400df/Objects/typeobject.c#L4867
This comment has been minimized.
This comment has been minimized.
thomasballinger
Aug 1, 2020
Member
If only dir() used getattr_static this would be ok; maybe we should reimplement dir() using getattr_static?
This comment has been minimized.
This comment has been minimized.
thomasballinger
Aug 1, 2020
Member
it'd be great to add a test like this for attr_matches to confirm a custom __getattribute__ isn't being invoked.
|
Thanks for the review. In addition to the small changes you mentioned, I added a test to test_autocomplete.py which ensures that an overridden As for |
|
Test Using it, I don't think anything's been broken. Test cleanup looks good. Good stuff! As expected, b.a. actually looks up a, using the custom |
rybarczykj commentedJul 30, 2020
I wanted to add open parenthesis completion in the suggestion box for callable functions, and I noticed there's some code we can consolidate or remove thanks to python3's
getattr_staticfunction. #163 mentions this. Here's what I changed.safe_get_attributeandsafe_get_attribute_new_stylefrom simpleeval.py, intoget_attr_safein inspection.py and move the tests from test_simpleeval to test_inspectionhas_attr_safeto inspection.pyOnce we drop python 2 support, we can delete the python2 definition for
get_attr_safeat which point AttrCleaner will be hardly used anywhere and can be removed / lightened up.