Skip to content

Commit 2367a21

Browse files
Copilotnathanjmcdougall
Copilot
and
nathanjmcdougall
authored
Respect Ruff rule code hierarchy in select/ignore lists (#1527)
* Initial plan * Add rule code hierarchy support to select_rules, ignore_rules, and ignore_rules_in_glob - Add is_rule_covered_by() and reconcile_rules() helpers to rule.py - Modify select_rules() and ignore_rules() in base.py to use hierarchy - Modify ignore_rules_in_glob() in ruff.py to use hierarchy - Add comprehensive tests for helper functions and integration behavior Co-authored-by: nathanjmcdougall <18602289+nathanjmcdougall@users.noreply.github.com> Agent-Logs-Url: https://github.com/usethis-python/usethis-python/sessions/880731be-ca47-4857-9975-4db0116718e6 * Fix ruff ERA001 lint error in test comments Co-authored-by: nathanjmcdougall <18602289+nathanjmcdougall@users.noreply.github.com> Agent-Logs-Url: https://github.com/usethis-python/usethis-python/sessions/880731be-ca47-4857-9975-4db0116718e6 * Replace tuple return from reconcile_rules with RuleReconciliation dataclass Co-authored-by: nathanjmcdougall <18602289+nathanjmcdougall@users.noreply.github.com> Agent-Logs-Url: https://github.com/usethis-python/usethis-python/sessions/f83674c7-4186-41aa-a4e8-babef65b8bb6 * Fix rule hierarchy bug: FLY is not a subrule of F, require digit boundary Co-authored-by: nathanjmcdougall <18602289+nathanjmcdougall@users.noreply.github.com> Agent-Logs-Url: https://github.com/usethis-python/usethis-python/sessions/94e8b825-bb91-4ef3-9763-5f4a59375e86 --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: nathanjmcdougall <18602289+nathanjmcdougall@users.noreply.github.com> Co-authored-by: Nathan McDougall <nathan.j.mcdougall@gmail.com>
1 parent c79b64c commit 2367a21

5 files changed

Lines changed: 438 additions & 30 deletions

File tree

‎src/usethis/_tool/base.py‎

Lines changed: 47 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
)
2020
from usethis._tool.config import NoConfigValue, ensure_managed_file_exists
2121
from usethis._tool.heuristics import is_likely_used
22+
from usethis._tool.rule import reconcile_rules
2223
from usethis._tool.spec import ToolMeta, ToolSpec
2324
from usethis._types.backend import BackendEnum
2425
from usethis.errors import (
@@ -436,28 +437,42 @@ def select_rules(self, rules: Sequence[Rule]) -> bool:
436437
These rules are not validated; it is assumed they are valid rules for the tool,
437438
and that the tool will be able to manage them.
438439
440+
Respects rule code hierarchy: if a more general rule is already selected, more
441+
specific rules will not be added. If a more general rule is being added, more
442+
specific existing rules will be removed.
443+
439444
Args:
440445
rules: The rules to select. If any of these rules are already selected, they
441446
will be skipped.
442447
443448
Returns:
444449
True if any rules were selected, False if no rules were selected.
445450
"""
446-
rules = sorted(set(rules) - set(self.selected_rules()))
451+
existing = self.selected_rules()
452+
reconciliation = reconcile_rules(existing=existing, incoming=list(rules))
447453

448-
if not rules:
454+
if reconciliation.is_noop:
449455
return False
450456

451-
rules_str = ", ".join([f"'{rule}'" for rule in rules])
452-
s = "" if len(rules) == 1 else "s"
453-
454457
(file_manager,) = self.get_active_config_file_managers()
455458
ensure_managed_file_exists(file_manager)
456-
tick_print(
457-
f"Selecting {self.name} rule{s} {rules_str} in '{file_manager.name}'."
458-
)
459459
keys = self._get_select_keys(file_manager)
460-
file_manager.extend_list(keys=keys, values=rules)
460+
461+
if reconciliation.to_remove:
462+
remove_str = ", ".join([f"'{rule}'" for rule in reconciliation.to_remove])
463+
s = "" if len(reconciliation.to_remove) == 1 else "s"
464+
tick_print(
465+
f"Deselecting {self.name} rule{s} {remove_str} in '{file_manager.name}'."
466+
)
467+
file_manager.remove_from_list(keys=keys, values=reconciliation.to_remove)
468+
469+
if reconciliation.to_add:
470+
add_str = ", ".join([f"'{rule}'" for rule in reconciliation.to_add])
471+
s = "" if len(reconciliation.to_add) == 1 else "s"
472+
tick_print(
473+
f"Selecting {self.name} rule{s} {add_str} in '{file_manager.name}'."
474+
)
475+
file_manager.extend_list(keys=keys, values=reconciliation.to_add)
461476

462477
return True
463478

@@ -489,28 +504,42 @@ def ignore_rules(self, rules: Sequence[Rule]) -> bool:
489504
These rules are not validated; it is assumed they are valid rules for the tool,
490505
and that the tool will be able to manage them.
491506
507+
Respects rule code hierarchy: if a more general rule is already ignored, more
508+
specific rules will not be added. If a more general rule is being added, more
509+
specific existing rules will be removed.
510+
492511
Args:
493512
rules: The rules to ignore. If any of these rules are already ignored, they
494513
will be skipped.
495514
496515
Returns:
497516
True if any rules were ignored, False if no rules were ignored.
498517
"""
499-
rules = sorted(set(rules) - set(self.ignored_rules()))
518+
existing = self.ignored_rules()
519+
reconciliation = reconcile_rules(existing=existing, incoming=list(rules))
500520

501-
if not rules:
521+
if reconciliation.is_noop:
502522
return False
503523

504-
rules_str = ", ".join([f"'{rule}'" for rule in rules])
505-
s = "" if len(rules) == 1 else "s"
506-
507524
(file_manager,) = self.get_active_config_file_managers()
508525
ensure_managed_file_exists(file_manager)
509-
tick_print(
510-
f"Ignoring {self.name} rule{s} {rules_str} in '{file_manager.name}'."
511-
)
512526
keys = self._get_ignore_keys(file_manager)
513-
file_manager.extend_list(keys=keys, values=rules)
527+
528+
if reconciliation.to_remove:
529+
remove_str = ", ".join([f"'{rule}'" for rule in reconciliation.to_remove])
530+
s = "" if len(reconciliation.to_remove) == 1 else "s"
531+
tick_print(
532+
f"No longer ignoring {self.name} rule{s} {remove_str} in '{file_manager.name}'."
533+
)
534+
file_manager.remove_from_list(keys=keys, values=reconciliation.to_remove)
535+
536+
if reconciliation.to_add:
537+
add_str = ", ".join([f"'{rule}'" for rule in reconciliation.to_add])
538+
s = "" if len(reconciliation.to_add) == 1 else "s"
539+
tick_print(
540+
f"Ignoring {self.name} rule{s} {add_str} in '{file_manager.name}'."
541+
)
542+
file_manager.extend_list(keys=keys, values=reconciliation.to_add)
514543

515544
return True
516545

‎src/usethis/_tool/impl/base/ruff.py‎

Lines changed: 26 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@
2323
)
2424
from usethis._tool.impl.spec.ruff import RuffToolSpec
2525
from usethis._tool.pre_commit import PreCommitConfig, PreCommitRepoConfig
26-
from usethis._tool.rule import Rule
26+
from usethis._tool.rule import Rule, reconcile_rules
2727
from usethis._types.backend import BackendEnum
2828

2929
if TYPE_CHECKING:
@@ -153,22 +153,37 @@ def ignored_rules(self) -> list[Rule]:
153153
return rules
154154

155155
def ignore_rules_in_glob(self, rules: Sequence[Rule], *, glob: str) -> None:
156-
"""Ignore Ruff rules in the project for a specific glob pattern."""
157-
rules = sorted(set(rules) - set(self.get_ignored_rules_in_glob(glob)))
156+
"""Ignore Ruff rules in the project for a specific glob pattern.
158157
159-
if not rules:
160-
return
158+
Respects rule code hierarchy: if a more general rule is already ignored for the
159+
glob, more specific rules will not be added. If a more general rule is being
160+
added, more specific existing rules will be removed.
161+
"""
162+
existing = self.get_ignored_rules_in_glob(glob)
163+
reconciliation = reconcile_rules(existing=existing, incoming=list(rules))
161164

162-
rules_str = ", ".join([f"'{rule}'" for rule in rules])
163-
s = "" if len(rules) == 1 else "s"
165+
if reconciliation.is_noop:
166+
return
164167

165168
(file_manager,) = self.get_active_config_file_managers()
166169
ensure_managed_file_exists(file_manager)
167-
tick_print(
168-
f"Ignoring {self.name} rule{s} {rules_str} for '{glob}' in '{file_manager.name}'."
169-
)
170170
keys = self._get_per_file_ignore_keys(file_manager, glob=glob)
171-
file_manager.extend_list(keys=keys, values=rules)
171+
172+
if reconciliation.to_remove:
173+
remove_str = ", ".join([f"'{rule}'" for rule in reconciliation.to_remove])
174+
s = "" if len(reconciliation.to_remove) == 1 else "s"
175+
tick_print(
176+
f"No longer ignoring {self.name} rule{s} {remove_str} for '{glob}' in '{file_manager.name}'."
177+
)
178+
file_manager.remove_from_list(keys=keys, values=reconciliation.to_remove)
179+
180+
if reconciliation.to_add:
181+
add_str = ", ".join([f"'{rule}'" for rule in reconciliation.to_add])
182+
s = "" if len(reconciliation.to_add) == 1 else "s"
183+
tick_print(
184+
f"Ignoring {self.name} rule{s} {add_str} for '{glob}' in '{file_manager.name}'."
185+
)
186+
file_manager.extend_list(keys=keys, values=reconciliation.to_add)
172187

173188
def unignore_rules_in_glob(self, rules: Sequence[Rule], *, glob: str) -> None:
174189
"""Stop ignoring Ruff rules in the project for a specific glob pattern."""

‎src/usethis/_tool/rule.py‎

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,107 @@
11
from __future__ import annotations
22

3+
from dataclasses import dataclass, field
34
from typing import TYPE_CHECKING, TypeAlias
45

56
from pydantic import BaseModel, Field
67
from typing_extensions import override
78

89
if TYPE_CHECKING:
10+
from collections.abc import Sequence
11+
912
from typing_extensions import Self
1013

1114
Rule: TypeAlias = str
1215

1316

17+
def is_rule_covered_by(rule: Rule, parent: Rule) -> bool:
18+
"""Check if a rule is covered (subsumed) by a more general rule.
19+
20+
A rule is covered if a more general rule would already include it.
21+
For example, "TC001" is covered by "TC", and any rule is covered by "ALL".
22+
23+
A rule does not cover itself. "ALL" is never covered by a specific rule.
24+
25+
Rule codes consist of a letter prefix (the group) followed by optional digits
26+
(the specific rule). A parent only covers a child if they share the same letter
27+
prefix; for example, "F" covers "F101" but not "FLY" or "FLY001".
28+
"""
29+
if parent == rule:
30+
return False
31+
if parent == "ALL":
32+
return True
33+
if rule == "ALL":
34+
return False
35+
if not rule.startswith(parent):
36+
return False
37+
# After the parent prefix, the next character in the child (if any) must be a
38+
# digit. This prevents "F" from covering "FLY" (next char 'L' is a letter,
39+
# meaning FLY is a separate rule group).
40+
rest = rule[len(parent) :]
41+
return rest == "" or rest[0].isdigit()
42+
43+
44+
@dataclass(frozen=True)
45+
class RuleReconciliation:
46+
"""Result of reconciling incoming rules with existing rules.
47+
48+
Attributes:
49+
to_add: Rules that should be added to the configuration.
50+
to_remove: Existing rules that are now subsumed and should be removed.
51+
"""
52+
53+
to_add: list[Rule] = field(default_factory=list)
54+
to_remove: list[Rule] = field(default_factory=list)
55+
56+
@property
57+
def is_noop(self) -> bool:
58+
"""Whether the reconciliation results in no changes."""
59+
return not self.to_add and not self.to_remove
60+
61+
62+
def reconcile_rules(
63+
existing: Sequence[Rule], incoming: Sequence[Rule]
64+
) -> RuleReconciliation:
65+
"""Determine which rules to add and which existing rules to remove.
66+
67+
Respects the rule code hierarchy: more general rules subsume more specific
68+
ones. For example, adding "TC" when "TC001" already exists will replace
69+
"TC001" with "TC". Adding "TC001" when "TC" already exists is a no-op.
70+
"""
71+
# Filter out incoming rules already covered by existing rules
72+
incoming_filtered: list[Rule] = []
73+
for rule in incoming:
74+
if rule in existing:
75+
continue
76+
if any(is_rule_covered_by(rule, e) for e in existing):
77+
continue
78+
incoming_filtered.append(rule)
79+
80+
# Among the filtered incoming rules, remove those covered by other incoming
81+
incoming_deduped: list[Rule] = []
82+
for rule in incoming_filtered:
83+
if any(
84+
is_rule_covered_by(rule, other)
85+
for other in incoming_filtered
86+
if other != rule
87+
):
88+
continue
89+
if rule not in incoming_deduped:
90+
incoming_deduped.append(rule)
91+
92+
# Determine which existing rules are now subsumed by incoming rules
93+
to_remove: list[Rule] = []
94+
for existing_rule in existing:
95+
if any(
96+
is_rule_covered_by(existing_rule, new_rule) for new_rule in incoming_deduped
97+
):
98+
to_remove.append(existing_rule)
99+
100+
return RuleReconciliation(
101+
to_add=sorted(incoming_deduped), to_remove=sorted(to_remove)
102+
)
103+
104+
14105
class RuleConfig(BaseModel):
15106
"""Configuration for linter rules associated with a tool.
16107

0 commit comments

Comments
 (0)