-
-
Notifications
You must be signed in to change notification settings - Fork 410
Rework WPS332: allow := in comprehensions #3129
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
"""We use this visitor to find walrus operators and ban them.""" | ||
def __init__(self, *args, **kwargs) -> None: | ||
super().__init__(*args, **kwargs) | ||
self._inside_comprehension = False |
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.
I think that it would be easier to use walk.get_closest_parent
and expect a ast.comprehension
@sobolevn next attempt. Removed ast.comprehension and added tuple with concrete comprehesions. Look likes clearly |
closest = walk.get_closest_parent( | ||
node, | ||
( | ||
ast.ListComp, |
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.
Please, move this to be a class level constant.
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.
Do you mean a whole tuple with possible comprehensions?
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.
Yes, like we do with other ClassVar[AnyNodes]
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.
Pushed :)
) | ||
) | ||
|
||
if closest is None: |
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.
Let's use early returns
if closest is None: | |
if closest is not None: | |
return |
) -> None: | ||
"""Disallows walrus ``:=`` operator.""" | ||
self.add_violation(consistency.WalrusViolation(node)) | ||
"""Disallows walrus ``:=`` operator outside comprehension.""" |
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.
"""Disallows walrus ``:=`` operator outside comprehension.""" | |
"""Disallows walrus ``:=`` operator outside comprehensions.""" |
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.
Great! Thank you!
Please, fix |
Hmmm... One moment. |
Merged all new changes from master. Can try again? =) |
"""Disallows walrus ``:=`` operator.""" | ||
self.add_violation(consistency.WalrusViolation(node)) | ||
"""Disallows walrus ``:=`` operator outside comprehensions.""" | ||
closest = walk.get_closest_parent( |
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.
Please, create a method for that. Let's call it _check_walrus_in_comprehension
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3129 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 117 117
Lines 6001 6007 +6
Branches 883 884 +1
=========================================
+ Hits 6001 6007 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
I have made things!
Checklist
I have double checked that there are no unrelated changes in this pull request (old patches, accidental config files, etc)
I have created at least one test case for the changes I have made
I have updated the documentation for the changes I have made
I have added my changes to the
CHANGELOG.md
Closes Rework
WPS332
: allow:=
in comprehensions #3121