From 58ce2f3bb8eaa49f4ac0d4d17f631fa393fb6cde Mon Sep 17 00:00:00 2001 From: sobolevn Date: Tue, 24 Dec 2024 18:52:33 +0300 Subject: [PATCH] Review --- CHANGELOG.md | 2 +- tests/fixtures/noqa/noqa.py | 5 ++ tests/test_checker/test_noqa.py | 2 +- .../test_compares/test_conditionals.py | 80 ++++++++++++++++++- .../test_pm/test_match_subjects.py | 36 ++++++--- .../logic/tree/compares.py | 3 +- .../logic/tree/pattern_matching.py | 24 ++++++ wemake_python_styleguide/options/defaults.py | 4 +- .../visitors/ast/compares.py | 19 ++++- .../visitors/ast/complexity/pm.py | 31 +++---- 10 files changed, 163 insertions(+), 43 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b13d9129a..3472b52f7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -101,7 +101,7 @@ Semantic versioning in our case means: - Adds a new rule to check problematic function params, #1343 - Adds a new rule to detect duplicate conditions in `if`s and `elif`s, #2241 - Adds a new rule to detect duplicate `case` pattens in `match`, #3206 -- Adds a new rule to find many `match` subjects, #3201 +- Adds a new rule to find too many `match` subjects, #3201 - Adds a new rule to find too complex `except` with too many exceptions - Adds a new rule to find too many `PEP695` type params - Adds a new rule to find useless ternary expressions, #1706 diff --git a/tests/fixtures/noqa/noqa.py b/tests/fixtures/noqa/noqa.py index f41000820..60e4264d1 100644 --- a/tests/fixtures/noqa/noqa.py +++ b/tests/fixtures/noqa/noqa.py @@ -631,6 +631,11 @@ def many_raises_function(parameter): # noqa: WPS238 my_print('except') +match inst1, inst2, inst3, inst4, inst5, inst6, inst7, inst8: # noqa: WPS241 + case 1: + my_print('except') + + my_print(""" text """) # noqa: WPS462 diff --git a/tests/test_checker/test_noqa.py b/tests/test_checker/test_noqa.py index ed77b6d73..b5a7437ae 100644 --- a/tests/test_checker/test_noqa.py +++ b/tests/test_checker/test_noqa.py @@ -92,7 +92,7 @@ 'WPS238': 1, 'WPS239': 1, 'WPS240': 0, # only triggers on 3.12+ - 'WPS241': 0, + 'WPS241': 1, 'WPS300': 1, 'WPS301': 1, 'WPS302': 0, # disabled since 1.0.0 diff --git a/tests/test_visitors/test_ast/test_compares/test_conditionals.py b/tests/test_visitors/test_ast/test_compares/test_conditionals.py index e828c9299..ffe985aa3 100644 --- a/tests/test_visitors/test_ast/test_compares/test_conditionals.py +++ b/tests/test_visitors/test_ast/test_compares/test_conditionals.py @@ -102,7 +102,6 @@ def test_valid_conditional( set_comprehension, dict_comprehension, gen_comprehension, - match_statement, ], ) @pytest.mark.parametrize( @@ -120,7 +119,8 @@ def test_valid_conditional( '{test : "1"}', '{"set"}', '("tuple",)', - '["list"]', + '[]', + '[variable]', 'variable or False', 'variable and False', 'variable or True', @@ -147,3 +147,79 @@ def test_constant_condition( visitor.run() assert_errors(visitor, [ConstantConditionViolation]) + + +@pytest.mark.parametrize( + 'comparators', + [ + 'True', + 'False', + 'None', + '4', + '-4.8', + '--0.0', + '"test"', + "b'bytes'", + '("string in brackets")', + '{1 : "1"}', + '{"set"}', + '("tuple",)', + '[]', + '[1, 2]', + 'variable or False', + 'variable and False', + 'variable or True', + 'variable and True', + '(unique := True)', + '(unique := -1)', + '...', + ], +) +def test_constant_condition_in_match( + assert_errors, + parse_ast_tree, + comparators, + default_options, + mode, +): + """Testing that violations are when using invalid conditional in PM.""" + tree = parse_ast_tree( + mode(create_variable.format(match_statement.format(comparators))), + ) + + visitor = WrongConditionalVisitor(default_options, tree=tree) + visitor.run() + + assert_errors(visitor, [ConstantConditionViolation]) + + +@pytest.mark.parametrize( + 'comparators', + [ + 'variable', + '(x := y)', + '-number', + '[x, y]', + '{test : "1"}', + '{test}', + '{**keys, "data": None}', + 'variable or other', + 'variable and other', + ], +) +def test_regular_condition_in_match( + assert_errors, + parse_ast_tree, + comparators, + default_options, + mode, +): + """Testing correct conditional in PM.""" + tree = parse_ast_tree( + mode(create_variable.format(match_statement.format(comparators))), + ) + + visitor = WrongConditionalVisitor(default_options, tree=tree) + visitor.run() + + assert_errors(visitor, []) diff --git a/tests/test_visitors/test_ast/test_complexity/test_pm/test_match_subjects.py b/tests/test_visitors/test_ast/test_complexity/test_pm/test_match_subjects.py index 25f16206d..4374fb4da 100644 --- a/tests/test_visitors/test_ast/test_complexity/test_pm/test_match_subjects.py +++ b/tests/test_visitors/test_ast/test_complexity/test_pm/test_match_subjects.py @@ -7,40 +7,50 @@ MatchSubjectsVisitor, ) -match_subjects9 = """ -match a, b, c, d, e, f, g, h, i: +match_call_expr = """ +match some_call(): case 1: ... """ + +match_case_long_tuple = """ +match some_call['a']: + case (a, b, c, d, e, f, g, h, i): ... +""" + match_subjects8 = """ -match a, b, c, d, e, f, g, h: +match a1, b2, c3, d4, e5, f6, g7, h8: + case 1: ... +""" + +match_subjects7 = """ +match a1, b2, c3, d4, e5, f6, g7: case 1: ... """ -@pytest.mark.parametrize( - 'code', - [match_subjects9], -) def test_match_subjects_wrong_count( assert_errors, assert_error_text, parse_ast_tree, - code, default_options, ): """Testing that default settings raise a warning.""" - tree = parse_ast_tree(code) + tree = parse_ast_tree(match_subjects8) visitor = MatchSubjectsVisitor(default_options, tree=tree) visitor.run() assert_errors(visitor, [TooManyMatchSubjectsViolation]) - assert_error_text(visitor, '9', baseline=default_options.max_match_subjects) + assert_error_text(visitor, '8', baseline=default_options.max_match_subjects) @pytest.mark.parametrize( 'code', - [match_subjects8], + [ + match_subjects7, + match_call_expr, + match_case_long_tuple, + ], ) def test_match_subjects_correct_count( assert_errors, @@ -60,8 +70,8 @@ def test_match_subjects_correct_count( @pytest.mark.parametrize( 'code', [ - match_subjects9, match_subjects8, + match_subjects7, ], ) def test_match_subjects_configured_count( @@ -73,7 +83,7 @@ def test_match_subjects_configured_count( """Testing that settings can reflect the change.""" tree = parse_ast_tree(code) - option_values = options(max_match_subjects=3) + option_values = options(max_match_subjects=1) visitor = MatchSubjectsVisitor(option_values, tree=tree) visitor.run() diff --git a/wemake_python_styleguide/logic/tree/compares.py b/wemake_python_styleguide/logic/tree/compares.py index b11c5cb44..4bb378210 100644 --- a/wemake_python_styleguide/logic/tree/compares.py +++ b/wemake_python_styleguide/logic/tree/compares.py @@ -2,10 +2,9 @@ import types from collections import defaultdict from collections.abc import Mapping -from typing import Final, TypeAlias +from typing import Final, TypeAlias, final import attr -from typing_extensions import final from wemake_python_styleguide.logic import source diff --git a/wemake_python_styleguide/logic/tree/pattern_matching.py b/wemake_python_styleguide/logic/tree/pattern_matching.py index 8c4f3937d..e7e638b69 100644 --- a/wemake_python_styleguide/logic/tree/pattern_matching.py +++ b/wemake_python_styleguide/logic/tree/pattern_matching.py @@ -2,7 +2,11 @@ from collections.abc import Iterable from wemake_python_styleguide.logic.nodes import get_parent +from wemake_python_styleguide.logic.tree import ( + operators, +) from wemake_python_styleguide.logic.walk import get_subnodes_by_type +from wemake_python_styleguide.logic.walrus import get_assigned_expr def get_explicit_as_names( @@ -21,3 +25,23 @@ def get_explicit_as_names( and match_as.name ): yield match_as + + +def is_constant_subject(condition: ast.AST | list[ast.expr]) -> bool: + """Detect constant subjects for `ast.Match` nodes.""" + if isinstance(condition, list): + return all(is_constant_subject(node) for node in condition) + node = operators.unwrap_unary_node(get_assigned_expr(condition)) + if isinstance(node, ast.Constant): + return True + if isinstance(node, ast.Tuple | ast.List | ast.Set): + return is_constant_subject(node.elts) + if isinstance(node, ast.Dict): + return ( + not any(dict_key is None for dict_key in node.keys) + and is_constant_subject([ + dict_key for dict_key in node.keys if dict_key is not None + ]) + and is_constant_subject(node.values) + ) + return False diff --git a/wemake_python_styleguide/options/defaults.py b/wemake_python_styleguide/options/defaults.py index f222c46ff..7d7bcfc44 100644 --- a/wemake_python_styleguide/options/defaults.py +++ b/wemake_python_styleguide/options/defaults.py @@ -138,10 +138,10 @@ MAX_TUPLE_UNPACK_LENGTH: Final = 4 # guessed #: Maximum number of PEP695 type parameters. -MAX_TYPE_PARAMS: Final = 6 # guessed +MAX_TYPE_PARAMS: Final = 6 # 7-1, guessed #: Maximum number of subjects in a ``match`` statement. -MAX_MATCH_SUBJECTS: Final = 8 # guessed +MAX_MATCH_SUBJECTS: Final = 7 # 7 +- 0, guessed # ========== # Formatter: diff --git a/wemake_python_styleguide/visitors/ast/compares.py b/wemake_python_styleguide/visitors/ast/compares.py index 481137406..084936ced 100644 --- a/wemake_python_styleguide/visitors/ast/compares.py +++ b/wemake_python_styleguide/visitors/ast/compares.py @@ -10,6 +10,7 @@ compares, functions, operators, + pattern_matching, ) from wemake_python_styleguide.logic.walrus import get_assigned_expr from wemake_python_styleguide.types import AnyIf, AnyNodes @@ -202,17 +203,27 @@ def visit_comprehension(self, node: ast.comprehension) -> None: def visit_Match(self, node: ast.Match) -> None: """Ensures that ``match`` nodes are using valid conditionals.""" - self._check_constant_condition(node.subject) + self._check_constant_condition(node.subject, is_match=True) self.generic_visit(node) - def _check_constant_condition(self, node: ast.AST) -> None: + def _check_constant_condition( + self, + node: ast.AST, + *, + is_match: bool = False, + ) -> None: if isinstance(node, ast.BoolOp): for condition in node.values: self._check_constant_condition(condition) else: real_node = operators.unwrap_unary_node(get_assigned_expr(node)) - if isinstance(real_node, self._forbidden_nodes): - self.add_violation(ConstantConditionViolation(node)) + if is_match and not pattern_matching.is_constant_subject(real_node): + return + if not is_match and not isinstance( + real_node, self._forbidden_nodes + ): + return + self.add_violation(ConstantConditionViolation(node)) def _check_nested_ifexpr(self, node: AnyIf) -> None: is_nested_in_if = bool( diff --git a/wemake_python_styleguide/visitors/ast/complexity/pm.py b/wemake_python_styleguide/visitors/ast/complexity/pm.py index d417b3f12..bd2076fa1 100644 --- a/wemake_python_styleguide/visitors/ast/complexity/pm.py +++ b/wemake_python_styleguide/visitors/ast/complexity/pm.py @@ -1,33 +1,28 @@ import ast - -from typing_extensions import final +from typing import final from wemake_python_styleguide.violations import complexity from wemake_python_styleguide.visitors.base import BaseNodeVisitor -from wemake_python_styleguide.visitors.decorators import alias @final -@alias( - 'visit_match', - ('visit_Match',), -) class MatchSubjectsVisitor(BaseNodeVisitor): """Finds excessive match subjects in `match` statements.""" - def visit_match(self, node: ast.Match) -> None: + def visit_Match(self, node: ast.Match) -> None: """Finds all `match` statements and checks their subjects.""" self._check_match_subjects_count(node) self.generic_visit(node) def _check_match_subjects_count(self, node: ast.Match) -> None: - if isinstance(node.subject, ast.Tuple): - elts = node.subject.elts - if len(elts) > self.options.max_match_subjects: - self.add_violation( - complexity.TooManyMatchSubjectsViolation( - node, - text=str(len(elts)), - baseline=self.options.max_match_subjects, - ) - ) + if not isinstance(node.subject, ast.Tuple): + return + if len(node.subject.elts) <= self.options.max_match_subjects: + return + self.add_violation( + complexity.TooManyMatchSubjectsViolation( + node, + text=str(len(node.subject.elts)), + baseline=self.options.max_match_subjects, + ) + )