From 8459040d5cbba8ce3917af69aa4d7d1b65d1907b Mon Sep 17 00:00:00 2001 From: shok9894 <31280079+shok9894@users.noreply.github.com> Date: Wed, 27 Mar 2019 23:12:13 +0300 Subject: [PATCH] Closes #506 (#526) --- tests/fixtures/noqa.py | 5 + tests/test_checker/test_noqa.py | 1 + .../test_multiline_conditions.py | 103 ++++++++++++++++++ .../violations/best_practices.py | 37 +++++++ .../visitors/ast/conditions.py | 12 ++ 5 files changed, 158 insertions(+) create mode 100644 tests/test_visitors/test_ast/test_conditions/test_multiline_conditions.py diff --git a/tests/fixtures/noqa.py b/tests/fixtures/noqa.py index 9467e9ac3..4b2e8e9b6 100644 --- a/tests/fixtures/noqa.py +++ b/tests/fixtures/noqa.py @@ -378,3 +378,8 @@ def bad_default_values( # noqa: Z459 raise TypeError('Second') except TypeError: print('WTF?') + +if some and ( # noqa: Z465 + anti_z444 == 1 +): + anti_z444 = 'some text' diff --git a/tests/test_checker/test_noqa.py b/tests/test_checker/test_noqa.py index 264ff61c6..444f89176 100644 --- a/tests/test_checker/test_noqa.py +++ b/tests/test_checker/test_noqa.py @@ -151,6 +151,7 @@ def test_noqa_fixture_disabled(absolute_path, all_violations): 'Z462': 1, 'Z463': 1, 'Z464': 1, + 'Z465': 1, } process = subprocess.Popen( diff --git a/tests/test_visitors/test_ast/test_conditions/test_multiline_conditions.py b/tests/test_visitors/test_ast/test_conditions/test_multiline_conditions.py new file mode 100644 index 000000000..1dad8c1c7 --- /dev/null +++ b/tests/test_visitors/test_ast/test_conditions/test_multiline_conditions.py @@ -0,0 +1,103 @@ +# -*- coding: utf-8 -*- + +import pytest + +from wemake_python_styleguide.violations.best_practices import ( + MultilineConditionsViolation, +) +from wemake_python_styleguide.visitors.ast.conditions import IfStatementVisitor + +incorrect_conditions1 = """if some and ( + other == 1 +): + ... +""" + +incorrect_conditions2 = """ +if some or ( + other == 1 +): + ... +""" + +incorrect_conditions3 = """if some and some_function( + other, +): + ... +""" + +incorrect_conditions4 = """ +if some or some_func( + other, +): + ... +""" + +incorrect_conditions5 = """ +if very_long_call_name( + long_parameter_name=long_variable_name, +): + ... +""" + +correct_conditions1 = """ +if some and other or something: + ... +""" + +correct_conditions2 = """ +if some_func(k) and (some or other): + ... +""" + +correct_conditions3 = """ +if (some_func(k) and some) or other in (1,2,3): + ... +""" + +correct_conditions4 = """ +if one: + if two: + ... +""" + + +@pytest.mark.parametrize('code', [ + incorrect_conditions1, + incorrect_conditions2, + incorrect_conditions3, + incorrect_conditions4, + incorrect_conditions5, +]) +def test_incorrect_multiline_conditions( + assert_errors, + parse_ast_tree, + code, + default_options, + mode, +): + """Testing multiline conditions.""" + tree = parse_ast_tree(mode(code)) + visitor = IfStatementVisitor(default_options, tree=tree) + visitor.run() + assert_errors(visitor, [MultilineConditionsViolation]) + + +@pytest.mark.parametrize('code', [ + correct_conditions1, + correct_conditions2, + correct_conditions3, + correct_conditions4, +]) +def test_correct_multiline_conditions( + assert_errors, + parse_ast_tree, + code, + default_options, + mode, +): + """Testing multiline conditions.""" + tree = parse_ast_tree(mode(code)) + visitor = IfStatementVisitor(default_options, tree=tree) + visitor.run() + assert_errors(visitor, []) diff --git a/wemake_python_styleguide/violations/best_practices.py b/wemake_python_styleguide/violations/best_practices.py index ab0e89ca8..e4953aa85 100644 --- a/wemake_python_styleguide/violations/best_practices.py +++ b/wemake_python_styleguide/violations/best_practices.py @@ -67,6 +67,7 @@ DirectMagicAttributeAccessViolation NegatedConditionsViolation NestedTryViolation + MultilineConditionsViolation Comments -------- @@ -130,6 +131,7 @@ .. autoclass:: DirectMagicAttributeAccessViolation .. autoclass:: NegatedConditionsViolation .. autoclass:: NestedTryViolation +.. autoclass:: MultilineConditionsViolation """ @@ -1799,3 +1801,38 @@ class NestedTryViolation(ASTViolation): error_template = 'Found nested `try` block' code = 464 + + +@final +class MultilineConditionsViolation(ASTViolation): + """ + Forbid multiline conditions. + + Reasoning: + This way of writing conditions hides the inner complexity this line has. + And it decreases readability of the code. + + Solution: + Divide multiline conditions to some ``if`` condition. Or use variables. + + Example:: + + # Correct: + if isinstance(node.test, ast.UnaryOp): + if isinstance(node.test.op, ast.Not): + ... + + + # Wrong: + if isinstance(node.test, ast.UnaryOp) and isinstance( + node.test.op, + ast.Not, + ): + ... + + .. versionadded:: 0.9.0 + + """ + + error_template = 'Found multiline conditions' + code = 465 diff --git a/wemake_python_styleguide/visitors/ast/conditions.py b/wemake_python_styleguide/visitors/ast/conditions.py index 48a3d5e7c..6bfc65261 100644 --- a/wemake_python_styleguide/visitors/ast/conditions.py +++ b/wemake_python_styleguide/visitors/ast/conditions.py @@ -5,6 +5,7 @@ from wemake_python_styleguide.types import AnyNodes, final from wemake_python_styleguide.violations.best_practices import ( + MultilineConditionsViolation, NegatedConditionsViolation, RedundantReturningElseViolation, ) @@ -33,6 +34,15 @@ def _check_negated_conditions(self, node: ast.If) -> None: if any(isinstance(elem, ast.NotEq) for elem in node.test.ops): self.add_violation(NegatedConditionsViolation(node)) + def _check_multiline_conditions(self, node: ast.If) -> None: + """Checks multiline conditions ``if`` statement nodes.""" + start_lineno = getattr(node, 'lineno', None) + for sub_nodes in ast.walk(node.test): + sub_lineno = getattr(sub_nodes, 'lineno', None) + if sub_lineno is not None and sub_lineno > start_lineno: + self.add_violation(MultilineConditionsViolation(node)) + break + def _check_redundant_else(self, node: ast.If) -> None: if not node.orelse: return @@ -52,8 +62,10 @@ def visit_If(self, node: ast.If) -> None: Raises: RedundantReturningElseViolation NegatedConditionsViolation + MultilineConditionsViolation """ self._check_negated_conditions(node) self._check_redundant_else(node) + self._check_multiline_conditions(node) self.generic_visit(node)