diff --git a/CHANGELOG.md b/CHANGELOG.md index 28a2cf0fa..cbaf42e29 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/tests/fixtures/noqa.py b/tests/fixtures/noqa.py index 92dc943e4..1a1d30b5f 100644 --- a/tests/fixtures/noqa.py +++ b/tests/fixtures/noqa.py @@ -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!') diff --git a/tests/test_checker/test_noqa.py b/tests/test_checker/test_noqa.py index a0bd9af12..60957120b 100644 --- a/tests/test_checker/test_noqa.py +++ b/tests/test_checker/test_noqa.py @@ -125,6 +125,7 @@ 'WPS349': 1, 'WPS350': 1, 'WPS351': 1, + 'WPS352': 1, 'WPS400': 0, 'WPS401': 0, diff --git a/tests/test_visitors/test_ast/test_loops/test_loops/test_multiline_loops.py b/tests/test_visitors/test_ast/test_loops/test_loops/test_multiline_loops.py new file mode 100644 index 000000000..b8069160b --- /dev/null +++ b/tests/test_visitors/test_ast/test_loops/test_loops/test_multiline_loops.py @@ -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, []) diff --git a/wemake_python_styleguide/violations/consistency.py b/wemake_python_styleguide/violations/consistency.py index 0b673d641..de1fdccc2 100644 --- a/wemake_python_styleguide/violations/consistency.py +++ b/wemake_python_styleguide/violations/consistency.py @@ -77,6 +77,7 @@ RedundantSubscriptViolation AugmentedAssignPatternViolation UnnecessaryLiteralsViolation + MultilineLoopViolation Consistency checks ------------------ @@ -133,6 +134,7 @@ .. autoclass:: RedundantSubscriptViolation .. autoclass:: AugmentedAssignPatternViolation .. autoclass:: UnnecessaryLiteralsViolation +.. autoclass:: MultilineLoopViolation """ @@ -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 diff --git a/wemake_python_styleguide/visitors/ast/loops.py b/wemake_python_styleguide/visitors/ast/loops.py index 41d8176d2..17ab7ddca 100644 --- a/wemake_python_styleguide/visitors/ast/loops.py +++ b/wemake_python_styleguide/visitors/ast/loops.py @@ -23,6 +23,7 @@ TooManyForsInComprehensionViolation, ) from wemake_python_styleguide.violations.consistency import ( + MultilineLoopViolation, MultipleIfsInComprehensionViolation, UselessContinueViolation, WrongLoopIterTypeViolation, @@ -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 @@ -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', (