Skip to content

Commit

Permalink
Added violation for multiline loops (#909)
Browse files Browse the repository at this point in the history
* Added violation for multiline loops

* Responded to feedback

* Fixes CI

* Fixes CI

* Fixes CI
  • Loading branch information
Ben-Drebing authored and sobolevn committed Oct 24, 2019
1 parent 46a4341 commit 9bb474b
Show file tree
Hide file tree
Showing 6 changed files with 148 additions and 0 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -29,6 +29,7 @@ We used to have incremental versioning before `0.1.0`.
- `WPS525` forbids comparisons where `in` is compared with single item container
- Fixes `BlockVariableVisitor` false positives on a properties
- Forbids wrong annotations in assignment
- Forbids using multiline `for` and `while` statements

### Bugfixes

Expand Down
5 changes: 5 additions & 0 deletions tests/fixtures/noqa.py
Expand Up @@ -580,5 +580,10 @@ def some_method(self):

int() # noqa: WPS351

for wrong_loop in call( # noqa: WPS352
1, 2, 3,
):
print('bad loop')

if a in {1}: # noqa: WPS525
print('bad!')
1 change: 1 addition & 0 deletions tests/test_checker/test_noqa.py
Expand Up @@ -125,6 +125,7 @@
'WPS349': 1,
'WPS350': 1,
'WPS351': 1,
'WPS352': 1,

'WPS400': 0,
'WPS401': 0,
Expand Down
@@ -0,0 +1,88 @@
# -*- coding: utf-8 -*-

import pytest

from wemake_python_styleguide.violations.consistency import (
MultilineLoopViolation,
)
from wemake_python_styleguide.visitors.ast.loops import WrongLoopVisitor

incorrect_loop1 = """
def wrapper():
for x in some_func(1,
3):
...
"""

incorrect_loop2 = """
def wrapper():
for x in (1,
2, 3, 4):
...
"""

incorrect_loop3 = """
while some_func(1,
3):
...
"""

correct_loop1 = """
def wrapper():
for x in (1, 2, 3, 4):
...
"""

correct_loop2 = """
def wrapper():
for x in (1, 2, 3, 4):
...
return
"""

correct_loop3 = """
while some_func(1,3):
...
"""


@pytest.mark.parametrize('code', [
incorrect_loop1,
incorrect_loop2,
incorrect_loop3,
])
def test_incorrect_multiline_loops(
assert_errors,
parse_ast_tree,
code,
default_options,
mode,
):
"""Testing multiline loops."""
tree = parse_ast_tree(mode(code))

visitor = WrongLoopVisitor(default_options, tree=tree)
visitor.run()

assert_errors(visitor, [MultilineLoopViolation])


@pytest.mark.parametrize('code', [
correct_loop1,
correct_loop2,
correct_loop3,
])
def test_correct_multiline_loops(
assert_errors,
parse_ast_tree,
code,
default_options,
mode,
):
"""Testing multiline loops."""
tree = parse_ast_tree(mode(code))

visitor = WrongLoopVisitor(default_options, tree=tree)
visitor.run()

assert_errors(visitor, [])
36 changes: 36 additions & 0 deletions wemake_python_styleguide/violations/consistency.py
Expand Up @@ -77,6 +77,7 @@
RedundantSubscriptViolation
AugmentedAssignPatternViolation
UnnecessaryLiteralsViolation
MultilineLoopViolation
Consistency checks
------------------
Expand Down Expand Up @@ -133,6 +134,7 @@
.. autoclass:: RedundantSubscriptViolation
.. autoclass:: AugmentedAssignPatternViolation
.. autoclass:: UnnecessaryLiteralsViolation
.. autoclass:: MultilineLoopViolation
"""

Expand Down Expand Up @@ -1904,3 +1906,37 @@ class UnnecessaryLiteralsViolation(ASTViolation):

error_template = 'Found unnecessary literals.'
code = 351


@final
class MultilineLoopViolation(ASTViolation):
"""
Forbids multiline loops.
Reasoning:
It decreased the readability of the code.
Solution:
Use single line loops and create new variables
in case you need to fit too many logic inside the loop definition.
Example::
# Correct
for num in some_function(arg1, arg2):
...
# Wrong
for num in range(
arg1,
arg2,
):
...
.. versionadded:: 0.13.0
"""

error_template = 'Forbids multiline loops'
code = 352
17 changes: 17 additions & 0 deletions wemake_python_styleguide/visitors/ast/loops.py
Expand Up @@ -23,6 +23,7 @@
TooManyForsInComprehensionViolation,
)
from wemake_python_styleguide.violations.consistency import (
MultilineLoopViolation,
MultipleIfsInComprehensionViolation,
UselessContinueViolation,
WrongLoopIterTypeViolation,
Expand Down Expand Up @@ -122,11 +123,13 @@ def visit_any_loop(self, node: _AnyLoop) -> None:
Raises:
UselessLoopElseViolation
LambdaInsideLoopViolation
MultilineLoopViolation
"""
self._check_loop_needs_else(node)
self._check_lambda_inside_loop(node)
self._check_useless_continue(node)
self._check_multiline_loop(node)
self.generic_visit(node)

def _does_loop_contain_node( # TODO: move, reuse in annotations.py
Expand Down Expand Up @@ -179,6 +182,20 @@ def _check_useless_continue(self, node: _AnyLoop) -> None:
if any(isinstance(last, ast.Continue) for last in last_line):
self.add_violation(UselessContinueViolation(node))

def _check_multiline_loop(self, node: _AnyLoop) -> None:
start_lineno = getattr(node, 'lineno', None)

if isinstance(node, ast.While):
node_to_check = node.test
else:
node_to_check = node.iter

for sub_node in ast.walk(node_to_check):
sub_lineno = getattr(sub_node, 'lineno', None)
if sub_lineno is not None and sub_lineno > start_lineno:
self.add_violation(MultilineLoopViolation(node))
break


@final
@decorators.alias('visit_any_for', (
Expand Down

0 comments on commit 9bb474b

Please sign in to comment.