Skip to content

Commit

Permalink
Closes #506 (#526)
Browse files Browse the repository at this point in the history
  • Loading branch information
SizovIgor authored and sobolevn committed Mar 27, 2019
1 parent b7d916d commit 8459040
Show file tree
Hide file tree
Showing 5 changed files with 158 additions and 0 deletions.
5 changes: 5 additions & 0 deletions tests/fixtures/noqa.py
Original file line number Diff line number Diff line change
Expand Up @@ -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'
1 change: 1 addition & 0 deletions tests/test_checker/test_noqa.py
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,7 @@ def test_noqa_fixture_disabled(absolute_path, all_violations):
'Z462': 1,
'Z463': 1,
'Z464': 1,
'Z465': 1,
}

process = subprocess.Popen(
Expand Down
Original file line number Diff line number Diff line change
@@ -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, [])
37 changes: 37 additions & 0 deletions wemake_python_styleguide/violations/best_practices.py
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@
DirectMagicAttributeAccessViolation
NegatedConditionsViolation
NestedTryViolation
MultilineConditionsViolation
Comments
--------
Expand Down Expand Up @@ -130,6 +131,7 @@
.. autoclass:: DirectMagicAttributeAccessViolation
.. autoclass:: NegatedConditionsViolation
.. autoclass:: NestedTryViolation
.. autoclass:: MultilineConditionsViolation
"""

Expand Down Expand Up @@ -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
12 changes: 12 additions & 0 deletions wemake_python_styleguide/visitors/ast/conditions.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

from wemake_python_styleguide.types import AnyNodes, final
from wemake_python_styleguide.violations.best_practices import (
MultilineConditionsViolation,
NegatedConditionsViolation,
RedundantReturningElseViolation,
)
Expand Down Expand Up @@ -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
Expand All @@ -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)

0 comments on commit 8459040

Please sign in to comment.