-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[pyupgrade] Fix false positive when class name is shadowed by local variable (UP008)
#20427
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
Conversation
|
ntBre
left a comment
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.
Thanks! I just had a minor simplification suggestion, but this looks good to me.
| if first_arg_id == parent_name.as_str() { | ||
| let scope = checker.semantic().current_scope(); | ||
| if scope.has(first_arg_id) { | ||
| return; | ||
| } | ||
| } |
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.
Can we combine this with the other first_arg_id == parent_name.as_str() check to avoid having to check that twice?
if !((first_arg_id == "__class__"
|| (first_arg_id == parent_name.as_str()
&& !checker.semantic().current_scope().has(first_arg_id)))
&& second_arg_id == parent_arg.name().as_str())
{
return;
}This condition is definitely getting a bit unwieldy, but I don't think it's too much worse.
An alternative approach here would be to resolve the binding of the first argument name and compare that to the binding of the parent class, but I think this check is okay because any intervening scope should also disable the rule. The current approach is a bit simpler.
ntBre
left a comment
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.
Thank you!
Summary
Fixes #20422