New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace setattr #493
Replace setattr #493
Conversation
NOTE: This also contains the changes from #492 and should be merged after that one is accepted. |
This PR can be merged after 1.6.1. I want 1.6.1 to contain just the regression bugs. |
@ericwb now that 1.6.2 is released with bug fixes, can we get a review on this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -211,7 +211,7 @@ def linerange(node): | |||
for key in strip.keys(): | |||
if hasattr(node, key): | |||
strip[key] = getattr(node, key) | |||
setattr(node, key, []) | |||
node.key = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is bug.
>>> key = "foo"
>>> setattr(node, key, [])
>>> node.foo
[]
>>> node.key
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
AttributeError: 'Node' object has no attribute 'key'
@@ -222,7 +222,7 @@ def linerange(node): | |||
|
|||
for key in strip.keys(): | |||
if strip[key] is not None: | |||
setattr(node, key, strip[key]) | |||
node.key = strip[key] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here too
setattr
doesn't provide any value over setting the attribute directly via___ = ___
. Also cleaned up a call toisinstance
where we were checking the same item twice in a single conditional.