Skip to content

Conversation

milssky
Copy link
Contributor

@milssky milssky commented Dec 12, 2024

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

@milssky milssky changed the title implement issue-3121 Rework WPS332: allow := in comprehensions Dec 12, 2024
"""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
Copy link
Member

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

@milssky
Copy link
Contributor Author

milssky commented Dec 12, 2024

@sobolevn next attempt. Removed ast.comprehension and added tuple with concrete comprehesions. Look likes clearly

@milssky milssky requested a review from sobolevn December 12, 2024 17:17
closest = walk.get_closest_parent(
node,
(
ast.ListComp,
Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

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]

Copy link
Contributor Author

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:
Copy link
Member

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

Suggested change
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."""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"""Disallows walrus ``:=`` operator outside comprehension."""
"""Disallows walrus ``:=`` operator outside comprehensions."""

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Thank you!

@sobolevn
Copy link
Member

Please, fix test_noqa.py and noqa.py fixture.

@milssky
Copy link
Contributor Author

milssky commented Dec 12, 2024

Please, fix test_noqa.py and noqa.py fixture.

Hmmm... One moment.

@milssky
Copy link
Contributor Author

milssky commented Dec 12, 2024

Merged all new changes from master. Can try again? =)

@sobolevn sobolevn closed this Dec 12, 2024
@sobolevn sobolevn reopened this Dec 12, 2024
"""Disallows walrus ``:=`` operator."""
self.add_violation(consistency.WalrusViolation(node))
"""Disallows walrus ``:=`` operator outside comprehensions."""
closest = walk.get_closest_parent(
Copy link
Member

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

Copy link

codecov bot commented Dec 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (cd5bd0b) to head (3b99435).
Report is 194 commits behind head on master.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sobolevn sobolevn merged commit 3bbcd8d into wemake-services:master Dec 13, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rework WPS332: allow := in comprehensions

2 participants