Skip to content

Commit a2bd044

Browse files
committed
ADD COP015 check for comprehension variable one_ prefix requirement
This new check ensures that variables in comprehensions and for-loops are prefixed with 'one_' to improve readability and consistency. It handles list, set, dict comprehensions and generator expressions, while properly ignoring underscore variables and partial unpacking scenarios.
1 parent 2859e79 commit a2bd044

3 files changed

Lines changed: 147 additions & 6 deletions

File tree

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
from __future__ import annotations
2+
import ast
3+
import typing
4+
5+
from community_of_python_flake8_plugin.constants import MIN_NAME_LENGTH
6+
from community_of_python_flake8_plugin.violation_codes import ViolationCodes
7+
from community_of_python_flake8_plugin.violations import Violation
8+
9+
10+
def _is_ignored_target(target_node: ast.expr) -> bool:
11+
"""Check if target should be ignored (e.g., underscore variables)."""
12+
# Ignore underscore variables
13+
return bool(isinstance(target_node, ast.Name) and target_node.id == "_")
14+
15+
16+
@typing.final
17+
class COP015ComprehensionOnePrefixCheck(ast.NodeVisitor):
18+
def __init__(self, syntax_tree: ast.AST) -> None: # noqa: ARG002
19+
self.violations: list[Violation] = []
20+
self.syntax_tree: typing.Final[ast.AST] = syntax_tree
21+
22+
def visit_ListComp(self, ast_node: ast.ListComp) -> None:
23+
# Validate targets in generators (the 'v' in 'for v in lst')
24+
for one_comprehension in ast_node.generators:
25+
if not self._is_partial_unpacking(ast_node.elt, one_comprehension.target):
26+
self._validate_comprehension_target(one_comprehension.target)
27+
self.generic_visit(ast_node)
28+
29+
def visit_SetComp(self, ast_node: ast.SetComp) -> None:
30+
# Validate targets in generators (the 'v' in 'for v in lst')
31+
for one_comprehension in ast_node.generators:
32+
if not self._is_partial_unpacking(ast_node.elt, one_comprehension.target):
33+
self._validate_comprehension_target(one_comprehension.target)
34+
self.generic_visit(ast_node)
35+
36+
def visit_DictComp(self, ast_node: ast.DictComp) -> None:
37+
# Validate targets in generators (the 'v' in 'for v in lst')
38+
# For dict comprehensions, we consider both key and value
39+
# key and value are both used
40+
for one_comprehension in ast_node.generators:
41+
if not self._is_partial_unpacking_expr_count(2, one_comprehension.target):
42+
self._validate_comprehension_target(one_comprehension.target)
43+
self.generic_visit(ast_node)
44+
45+
def visit_For(self, ast_node: ast.For) -> None:
46+
# Validate target variables in regular for-loops
47+
# Apply same unpacking logic as comprehensions
48+
# For-loops don't have an expression that references vars
49+
if not self._is_partial_unpacking_expr_count(1, ast_node.target):
50+
self._validate_comprehension_target(ast_node.target)
51+
self.generic_visit(ast_node)
52+
53+
def visit_GeneratorExp(self, ast_node: ast.GeneratorExp) -> None:
54+
# Validate targets in generators (the 'v' in 'for v in lst')
55+
for one_comprehension in ast_node.generators:
56+
if not self._is_partial_unpacking(ast_node.elt, one_comprehension.target):
57+
self._validate_comprehension_target(one_comprehension.target)
58+
self.generic_visit(ast_node)
59+
60+
def _is_partial_unpacking(self, expression: ast.expr, target_node: ast.expr) -> bool:
61+
"""Check if this is partial unpacking (referencing fewer vars than unpacked)."""
62+
return self._is_partial_unpacking_expr_count(self._count_referenced_vars(expression), target_node)
63+
64+
def _is_partial_unpacking_expr_count(self, expression_count: int, target_node: ast.expr) -> bool:
65+
"""Check if this is partial unpacking given expression variable count."""
66+
target_count: typing.Final = self._count_unpacked_vars(target_node)
67+
return target_count > expression_count and target_count > 1
68+
69+
def _count_referenced_vars(self, expression: ast.expr) -> int:
70+
"""Count how many variables are referenced in the expression."""
71+
if isinstance(expression, ast.Name):
72+
return 1
73+
if isinstance(expression, ast.Tuple):
74+
return len([one_element for one_element in expression.elts if isinstance(one_element, ast.Name)])
75+
# For simplicity, assume other expressions reference 1 variable
76+
# This covers cases like function calls, attributes, etc.
77+
return 1
78+
79+
def _count_unpacked_vars(self, target_node: ast.expr) -> int:
80+
"""Count how many variables are unpacked in the target."""
81+
if isinstance(target_node, ast.Name):
82+
return 1
83+
if isinstance(target_node, ast.Tuple):
84+
return len([one_element for one_element in target_node.elts if isinstance(one_element, ast.Name)])
85+
return 0
86+
87+
def _validate_comprehension_target(self, target_node: ast.expr) -> None:
88+
"""Validate that comprehension target follows the one_ prefix rule."""
89+
# Skip ignored targets (underscore, unpacking)
90+
if _is_ignored_target(target_node):
91+
return
92+
93+
# For simple names, validate the prefix
94+
if isinstance(target_node, ast.Name):
95+
if not self._has_valid_one_prefix(target_node.id):
96+
self.violations.append(
97+
Violation(
98+
line_number=target_node.lineno,
99+
column_number=target_node.col_offset,
100+
violation_code=ViolationCodes.COMPREHENSION_VARIABLE_PREFIX,
101+
)
102+
)
103+
# For tuples (unpacking), validate each element
104+
elif isinstance(target_node, ast.Tuple):
105+
for one_element in target_node.elts:
106+
if isinstance(one_element, ast.Name) and not self._has_valid_one_prefix(one_element.id):
107+
self.violations.append(
108+
Violation(
109+
line_number=one_element.lineno,
110+
column_number=one_element.col_offset,
111+
violation_code=ViolationCodes.COMPREHENSION_VARIABLE_PREFIX,
112+
)
113+
)
114+
115+
def _has_valid_one_prefix(self, identifier: str) -> bool:
116+
"""Check if identifier has valid one_ prefix."""
117+
# Allow underscore variables
118+
if identifier == "_":
119+
return True
120+
121+
# Check for one_ prefix
122+
return identifier.startswith("one_")

src/community_of_python_flake8_plugin/violation_codes.py

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,3 +47,8 @@ class ViolationCodes:
4747
DATACLASS_CONFIG = ViolationCodeItem(
4848
code="COP014", description="Use dataclasses with kw_only=True, slots=True, frozen=True"
4949
)
50+
51+
# Comprehension variable prefix violations
52+
COMPREHENSION_VARIABLE_PREFIX = ViolationCodeItem(
53+
code="COP015", description="Comprehension variables must be prefixed with 'one_'"
54+
)

tests/test_plugin.py

Lines changed: 20 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -483,12 +483,6 @@ def test_dataclass_validations(input_source: str, expected_output: list[str]) ->
483483
("class ExampleClass:\n a: int = 1", ["COP012", "COP004"]),
484484
# COP004: Class-level Final assignment should still be treated as attribute
485485
("class ExampleClass:\n client: typing.Final = CreditStatementClient()", ["COP012", "COP004"]),
486-
# COP005: List comprehension with short variable names should be flagged
487-
("result_long_name = [v for v in some_list]", ["COP005"]),
488-
# No violation: List comprehension with long variable names
489-
("result_long_name = [value for value in some_list]", []),
490-
# No violation: List comprehension with underscore variable
491-
("result_long_name = [_ for _ in some_list]", []),
492486
# COP005: With statement with short variable name should be flagged
493487
("with open('file.txt') as f: pass", ["COP005"]),
494488
# No violation: With statement with long variable name
@@ -503,6 +497,26 @@ def test_dataclass_validations(input_source: str, expected_output: list[str]) ->
503497
("func_long_name = lambda value: value * 2", []),
504498
# COP006: Lambda with multiple short arguments should be flagged
505499
("func_long_name = lambda a, b: a + b", ["COP006", "COP006"]),
500+
# COP015: List comprehension without one_ prefix should be flagged (in addition to COP005)
501+
("result_long_name = [v for v in some_list]", ["COP005", "COP015"]),
502+
# COP005: List comprehension with one_ prefix (still too short for general rule)
503+
("result_long_name = [one_v for one_v in some_list]", ["COP005"]),
504+
# No violation: List comprehension with underscore (ignored)
505+
("result_long_name = [_ for _ in some_list]", []),
506+
# COP005: List comprehension with tuple unpacking (prefix rule ignored, but length rule still applies)
507+
("result_long_name = [x for x, y in pairs]", ["COP005", "COP005"]),
508+
# COP015: Set comprehension without one_ prefix should be flagged (in addition to COP005)
509+
("result_long_name = {v for v in some_list}", ["COP005", "COP015"]),
510+
# COP005: Set comprehension with one_ prefix (vars still too short for general rule)
511+
("result_long_name = {one_v for one_v in some_list}", ["COP005"]),
512+
# COP005+COP015: Dict comprehension without one_ prefix should be flagged
513+
("result_long_name = {k: v for k, v in pairs}", ["COP005", "COP005", "COP015", "COP015"]),
514+
# COP005: Dict comprehension with one_ prefix (vars still too short for general rule)
515+
("result_long_name = {one_k: one_v for one_k, one_v in pairs}", ["COP005", "COP005"]),
516+
# COP015: Generator expression without one_ prefix should be flagged (in addition to COP005)
517+
("result_long_name = (v for v in some_list)", ["COP005", "COP015"]),
518+
# COP005: Generator expression with one_ prefix (vars still too short for general rule)
519+
("result_long_name = (one_v for one_v in some_list)", ["COP005"]),
506520
],
507521
)
508522
def test_module_vs_class_level_assignments(input_source: str, expected_output: list[str]) -> None:

0 commit comments

Comments
 (0)