From c0c07c3c8b91e9bbf4a70fd1efcd58462cb4da64 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 27 Sep 2026 23:23:50 +0000 Subject: [PATCH 1/3] feat: make every sub-PR work on its own after planning A plan could leave a sub-PR needing code another sub-PR adds. Splitting this repo's own 4,187-line merge with the graph backend, 18 of 19 sub-PRs failed their own test suite. After any backend plans, a new pass (planner/coherence.py) finds what each group needs and fixes the plan: - a group depends on every group whose new names it uses, on the group holding an added import it relies on, on the code filling a new field it reads, and on the group adding a name its mock.patch target names; - tests are separated from unrelated code; a test group depends on the changed modules its tests import (followed through unchanged modules), files it loads by path, and its conftest, and rejoins the code it tests where --max-loc allows; - edits to existing tests, conftest changes and new autouse fixtures move into the group whose code change they follow, so that group's own PR still passes its old tests; - hunks of one function stay together, as does a lockfile with its manifest; - groups that end up needing each other are combined, with the reason logged, instead of the cycle-closing dependency being dropped. Symbol analysis also stops linking on false matches: names a test file defines no longer tie to other files, and edited (not new) definitions and multi-line signature parameters no longer count as definitions. Graph and CP-SAT groups get titles that say what they change ("Update pr_split/cli.py (part 1 of 2) and 3 more files") instead of "chore(split): review cli-1". With this, all 18 sub-PRs of that merge and all 10 of an older 1,698-line feature PR pass their full test suite on their own, and merging each set back reproduces the original branch exactly. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015rnEEjXWDBdePRhKMmZr1w --- README.md | 22 +- pr_split/cli.py | 5 +- pr_split/diff_ops/__init__.py | 8 +- pr_split/diff_ops/reconstructor.py | 8 + pr_split/logs.py | 10 +- pr_split/planner/client.py | 29 +- pr_split/planner/coherence.py | 972 +++++++++++++++++++++++++++ pr_split/planner/partitioning.py | 124 ++-- pr_split/planner/symbols.py | 212 +++++- tests/test_coherence.py | 357 ++++++++++ tests/test_partitioning_extensive.py | 4 +- tests/test_symbols.py | 123 +++- 12 files changed, 1761 insertions(+), 113 deletions(-) create mode 100644 pr_split/planner/coherence.py create mode 100644 tests/test_coherence.py diff --git a/README.md b/README.md index 261a160..912a7e8 100644 --- a/README.md +++ b/README.md @@ -378,11 +378,23 @@ For a deeper explanation of the planning model, optimization methods, scoring, a 1. Extracts the merge-base diff between your branch and the base (same view as GitHub's PR page) 2. Sends the diff to the configured backend (LLM, graph, or CP-SAT), which groups hunks into logical sub-PRs with dependency ordering -3. Validates the plan: full coverage (every hunk assigned exactly once), no cycles, no merge conflicts between independent groups -4. Shows you the plan (table + dependency tree) with an optional interactive editor to move hunks between groups -5. Creates branches, commits, pushes, and opens GitHub PRs — materialization and push/PR creation run in parallel using git worktrees. Use `--dry-run` to save the plan for later execution with `execute` -6. For diffs exceeding the model's context window, uses the configured chunking strategy and processes chunks sequentially while carrying forward the group catalog across chunks -7. `status`, `merge`, and `clean` commands are available to track progress, merge PRs in dependency order, or clean up branches and PRs +3. Makes each sub-PR work on its own, whichever backend planned it (see [Standalone sub-PRs](#standalone-sub-prs)) +4. Validates the plan: full coverage (every hunk assigned exactly once), no cycles, no merge conflicts between independent groups +5. Shows you the plan (table + dependency tree) with an optional interactive editor to move hunks between groups +6. Creates branches, commits, pushes, and opens GitHub PRs — materialization and push/PR creation run in parallel using git worktrees. Use `--dry-run` to save the plan for later execution with `execute` +7. For diffs exceeding the model's context window, uses the configured chunking strategy and processes chunks sequentially while carrying forward the group catalog across chunks +8. `status`, `merge`, and `clean` commands are available to track progress, merge PRs in dependency order, or clean up branches and PRs + +### Standalone sub-PRs + +A backend groups hunks by size and affinity, which can leave a sub-PR needing code another sub-PR adds; its CI would then fail on its own. After planning, pr-split finds what each group needs and fixes the plan: + +- **Dependencies it uses.** A group depends on every group whose new functions, classes, constants or fields it uses. It also depends on the group holding an added import it relies on, on the code that fills a new field it reads, and on the group that adds a name its `mock.patch("pkg.mod.name")` targets. +- **Tests after the code they test.** Tests are taken out of groups of unrelated code. A test group depends on every changed module its tests import (followed through unchanged modules), on the files it loads by path, and on its `conftest.py`. Where the size limit allows, a test rejoins the group whose code it tests. +- **Test updates with the change they follow.** An edit to an existing test, a conftest change, or a new autouse fixture moves into the group whose code change it adapts to. Otherwise the old test would fail in that group's own PR. +- **Pieces that only work together.** Hunks of one function or its decorators stay in one group, as does a lockfile with its manifest (`uv.lock` and `pyproject.toml`, `package-lock.json` and `package.json`, …). + +Groups that end up needing each other are combined, and each combination is logged with the reason. The combined PR may exceed `--max-loc`; with `--strict-loc-bounds` that is an error, as for any oversized group. Import analysis is for Python; for other languages tests are paired with the file they are named after. The analysis is static, so behaviour a test only exercises indirectly can still slip through: check the sub-PRs' CI. ## License diff --git a/pr_split/cli.py b/pr_split/cli.py index 31f1094..89d4cb1 100644 --- a/pr_split/cli.py +++ b/pr_split/cli.py @@ -11,6 +11,7 @@ import urllib.request from collections.abc import Callable, Generator from concurrent.futures import ThreadPoolExecutor, as_completed +from functools import partial from pathlib import Path from threading import Lock, Semaphore from typing import Annotated @@ -46,6 +47,7 @@ materialize_group_files, merge_chain_assignments, parse_diff, + read_file_at, target_file_modes, ) from .exceptions import ( @@ -1646,7 +1648,8 @@ def split( console.print(f"[red]{exc}[/red]") raise typer.Exit(1) from exc try: - groups = plan_split(parsed_diff, settings) + # Tests' full imports are read at the dev head to find what they exercise. + groups = plan_split(parsed_diff, settings, read_file=partial(read_file_at, dev_branch)) link_new_file_pieces(groups, parsed_diff) except PRSplitError as exc: console.print(f"[red]{exc}[/red]") diff --git a/pr_split/diff_ops/__init__.py b/pr_split/diff_ops/__init__.py index 42794c1..27c2e6a 100644 --- a/pr_split/diff_ops/__init__.py +++ b/pr_split/diff_ops/__init__.py @@ -1,5 +1,10 @@ from .parser import ParsedDiff, extract_diff, parse_diff -from .reconstructor import materialize_group_files, merge_chain_assignments, target_file_modes +from .reconstructor import ( + materialize_group_files, + merge_chain_assignments, + read_file_at, + target_file_modes, +) __all__ = [ "ParsedDiff", @@ -7,5 +12,6 @@ "materialize_group_files", "merge_chain_assignments", "parse_diff", + "read_file_at", "target_file_modes", ] diff --git a/pr_split/diff_ops/reconstructor.py b/pr_split/diff_ops/reconstructor.py index 82fea99..9122361 100644 --- a/pr_split/diff_ops/reconstructor.py +++ b/pr_split/diff_ops/reconstructor.py @@ -72,6 +72,14 @@ def _get_base_file_content(file_path: str, ref: str) -> str: return result.stdout.decode("utf-8", errors="surrogateescape") +def read_file_at(ref: str, file_path: str) -> str | None: + """A file's content at ``ref``, or None if it does not exist there.""" + try: + return _get_base_file_content(file_path, ref) + except GitOperationError: + return None + + NO_NEWLINE_MARKER = "\\" diff --git a/pr_split/logs.py b/pr_split/logs.py index f767a24..825f815 100644 --- a/pr_split/logs.py +++ b/pr_split/logs.py @@ -157,9 +157,13 @@ "The split's base branch {base} has already merged; run 'pr-split retarget' to move" " the split onto the new base before merging" ) -SYMBOL_EDGE_SKIPPED = ( - "Group '{user}' uses {names} from group '{definer}', but depending on it would" - " create a cycle; the edge was not added" +TEST_UPDATES_ATTACHED = ( + "Moved {count} changed existing test hunk(s) into the PR whose code change they follow," + " so that PR's own tests still pass" +) +GROUPS_COMBINED = ( + "Combined {others} into {keep}: they need each other's code, so neither would work" + " on its own ({reasons})" ) SYMBOL_ORDER_VIOLATION = ( "Group '{user}' uses {names} defined in group '{definer}', which it does not build on;" diff --git a/pr_split/planner/client.py b/pr_split/planner/client.py index 2b1ccd4..59c7954 100644 --- a/pr_split/planner/client.py +++ b/pr_split/planner/client.py @@ -3,7 +3,7 @@ import ipaddress import json import subprocess -from typing import TypedDict +from typing import TYPE_CHECKING, TypedDict from urllib.parse import urlsplit import anthropic @@ -36,7 +36,8 @@ format_group_catalog, recompute_estimated_loc, ) -from .partitioning import partition_diff +from .coherence import make_groups_standalone +from .partitioning import describe_files, partition_diff, retitle_groups from .prompts import ( SPLIT_TOOL_NAME, SPLIT_TOOL_SCHEMA, @@ -50,6 +51,9 @@ from .scoring import score_plan from .validator import detect_loc_bound_violations, validate_coverage, validate_no_conflicts +if TYPE_CHECKING: + from collections.abc import Callable + _ANTHROPIC_TOOL_DEF = anthropic.types.ToolParam( name=SPLIT_TOOL_NAME, description="Propose a plan to split the diff into groups", @@ -742,7 +746,14 @@ def _single_group_plan(parsed_diff: ParsedDiff) -> list[Group]: def plan_split( parsed_diff: ParsedDiff, settings: Settings, + read_file: Callable[[str], str | None] | None = None, ) -> list[Group]: + """Plan the split, then make each group a sub-PR that works on its own. + + ``read_file`` returns a file as it stands at the dev branch's head (None + if absent); with it, a test's full imports are followed, not just the + lines its hunks show. + """ if settings.partition_strategy not in ( PartitionStrategy.LLM, PartitionStrategy.GRAPH, @@ -760,6 +771,20 @@ def plan_split( groups = _plan_split_with_llm(parsed_diff, settings) else: groups = partition_diff(parsed_diff, settings) + groups = make_groups_standalone( + groups, + parsed_diff, + read_file, + settings.max_loc, + keep_declared_deps=settings.partition_strategy is PartitionStrategy.LLM, + ) + if settings.partition_strategy is not PartitionStrategy.LLM: + # Combining groups changed what some hold; name them after it again. + retitle_groups(groups, parsed_diff) + for group in groups: + group.description = describe_files( + settings.partition_strategy.value, (a.file_path for a in group.assignments) + ) metrics = score_plan(groups, settings.max_loc, settings.min_loc) logger.info( diff --git a/pr_split/planner/coherence.py b/pr_split/planner/coherence.py new file mode 100644 index 0000000..8875176 --- /dev/null +++ b/pr_split/planner/coherence.py @@ -0,0 +1,972 @@ +"""Make every group a sub-PR that works on its own. + +A backend packs hunks into groups by size and affinity, which can leave a +group needing code another group adds: a symbol it uses, the module its +tests import, a ``patch("pkg.mod.name")`` target, a conftest fixture, or a +lockfile's manifest. ``make_groups_standalone`` fixes the plan in steps: + +1. Tests are separated from groups of unrelated code (``_peel_tests``). +2. Each group gets a dependency on every group whose code it needs + (``find_needs``); groups that need each other form a cycle no order can + satisfy, so they are combined into one (``_condense``). +3. Edits to existing tests move to the group whose code change they follow, + or that group's own PR would fail its old tests + (``_attach_test_updates``, ``_carry_imports``); needs are read again. +4. Each remaining test group rejoins the code it tests where that fits + within the size limit (``_rehome_tests``). + +A bigger PR that works beats two smaller ones that each fail on their own. +""" + +from __future__ import annotations + +import ast +import functools +import itertools +import re +from collections import defaultdict +from dataclasses import dataclass +from graphlib import CycleError, TopologicalSorter +from pathlib import PurePosixPath +from typing import TYPE_CHECKING + +from loguru import logger + +from .. import logs +from ..constants import AssignmentType +from ..schemas import GroupAssignment +from .chunker import recompute_estimated_loc +from .partitioning import derive_file_order_dependencies, reduce_transitive_dependencies +from .symbols import ( + LOCKFILE_MANIFESTS, + added_lines_by_file, + import_bindings, + is_test_path, + symbol_dependencies, +) + +if TYPE_CHECKING: + from collections.abc import Callable, Iterable + + from unidiff import Hunk + + from ..diff_ops import ParsedDiff + from ..schemas import Group + +# A dotted string such as "pkg.mod.name": a mock.patch target, or an import path. +_DOTTED_STRING = re.compile(r"""["']([A-Za-z_]\w*(?:\.[A-Za-z_]\w*)+)["']""") +_IDENTIFIER = re.compile(r"[A-Za-z_]\w*") +_MIN_WORD_LENGTH = 4 +# Words too common in code to tie a test to one change. +_COMMON_WORDS = frozenset( + { + "self", + "None", + "True", + "False", + "return", + "assert", + "import", + "from", + "with", + "patch", + "mock", + "MagicMock", + "call", + "args", + "kwargs", + "result", + "tests", + "test", + "def", + "class", + "str", + "int", + "list", + "dict", + "value", + "path", + "name", + "output", + "exit_code", + "pytest", + "raises", + "assert_called_once_with", + "assert_not_called", + "return_value", + "side_effect", + } +) +_FROM_IMPORT = re.compile(r"^\s*from\s+(\.*[\w.]*)\s+import\s+\(?([\w\s,*]+)") +_PLAIN_IMPORT = re.compile(r"^\s*import\s+([\w.]+(?:\s*,\s*[\w.]+)*)") + + +@dataclass(frozen=True) +class Need: + """``user`` cannot work without code that ``provider`` adds.""" + + user: str + provider: str + reason: str + + +def _hunk_lines(parsed_diff: ParsedDiff, path: str) -> list[str]: + """Added and context lines of a file's hunks: its imports, as far as the diff shows.""" + for patch_file in parsed_diff.patch_set: + if patch_file.path == path: + return [ + line.value.rstrip("\n") + for hunk in patch_file + for line in hunk + if line.is_added or line.is_context + ] + return [] + + +def _module_files(module: str) -> tuple[str, str]: + base = module.replace(".", "/") + return f"{base}.py", f"{base}/__init__.py" + + +def _resolve_relative(path: str, module: str) -> str: + """Turn ``from .x import y`` in ``path`` into an absolute dotted module.""" + level = len(module) - len(module.lstrip(".")) + if not level: + return module + package = list(PurePosixPath(path).parent.parts) + package = package[: len(package) - (level - 1)] if level > 1 else package + rest = module[level:] + return ".".join([*package, rest] if rest else package) + + +def _imported_modules(path: str, source: str) -> list[tuple[str, list[str]]]: + """(module, imported names) for each import in a Python source.""" + found: list[tuple[str, list[str]]] = [] + try: + tree = ast.parse(source) + except (SyntaxError, ValueError): + # A partial view (hunks only) is rarely valid Python; read it line by line. + for line in source.splitlines(): + if match := _FROM_IMPORT.match(line): + names = [n.strip().split(" as ")[0] for n in match.group(2).split(",")] + found.append((_resolve_relative(path, match.group(1)), [n for n in names if n])) + elif match := _PLAIN_IMPORT.match(line): + found.extend((m.strip(), []) for m in match.group(1).split(",")) + return found + for node in ast.walk(tree): + if isinstance(node, ast.ImportFrom): + module = "." * node.level + (node.module or "") + found.append((_resolve_relative(path, module), [a.name for a in node.names])) + elif isinstance(node, ast.Import): + found.extend((alias.name, []) for alias in node.names) + return found + + +class _ImportGraph: + """Python files reachable through imports, read at the dev branch's head.""" + + def __init__( + self, parsed_diff: ParsedDiff, read_file: Callable[[str], str | None] | None + ) -> None: + self._parsed_diff = parsed_diff + self._read_file = read_file + self._changed = {pf.path for pf in parsed_diff.patch_set} + self._exists: dict[str, bool] = {} + self._imports: dict[str, list[str]] = {} + + def _source(self, path: str) -> str | None: + if self._read_file is not None: + text = self._read_file(path) + if text is not None: + return text + if path in self._changed: + return "\n".join(_hunk_lines(self._parsed_diff, path)) + return None + + def text(self, path: str) -> str: + return self._source(path) or "" + + def _is_file(self, path: str) -> bool: + if path in self._changed: + return True + if self._read_file is None: + return False + if path not in self._exists: + self._exists[path] = self._read_file(path) is not None + return self._exists[path] + + def direct_imports(self, path: str) -> list[str]: + if path not in self._imports: + source = self._source(path) if path.endswith(".py") else None + targets: list[str] = [] + for module, names in _imported_modules(path, source or ""): + if not module: + continue + for candidate in _module_files(module): + if self._is_file(candidate): + targets.append(candidate) + break + for name in names: + for candidate in _module_files(f"{module}.{name}"): + if self._is_file(candidate): + targets.append(candidate) + break + self._imports[path] = list(dict.fromkeys(targets)) + return self._imports[path] + + def direct_targets(self, path: str) -> set[str]: + """Files a test imports, names in a string or patch target, or is named after.""" + text = self.text(path) + stem = PurePosixPath(path).stem.removeprefix("test_").removesuffix("_test") + named = { + changed + for changed in self._changed + if re.search(rf"""["'/]{re.escape(PurePosixPath(changed).name)}["']""", text) + or PurePosixPath(changed).stem == stem + } + patched = { + target[0] + for dotted in _DOTTED_STRING.findall(text) + if (target := _patch_target(dotted, self._changed)) is not None + } + return set(self.direct_imports(path)) | named | patched + + def reachable(self, path: str) -> set[str]: + """Every repository file ``path`` imports, directly or through other modules.""" + seen: set[str] = set() + pending = [path] + while pending: + for target in self.direct_imports(pending.pop()): + if target not in seen and target != path: + seen.add(target) + pending.append(target) + return seen + + +def _groups_by_file(groups: list[Group]) -> dict[str, list[str]]: + by_file: dict[str, list[str]] = defaultdict(list) + for group in groups: + for assignment in group.assignments: + if group.id not in by_file[assignment.file_path]: + by_file[assignment.file_path].append(group.id) + return by_file + + +def _symbol_needs(groups: list[Group], parsed_diff: ParsedDiff) -> Iterable[Need]: + for user, definers in symbol_dependencies(groups, parsed_diff).items(): + for provider, names in definers.items(): + shown = ", ".join(sorted(names)[:3]) + yield Need(user, provider, f"{user} uses {shown} from {provider}") + + +def _patch_target(dotted: str, changed: Iterable[str]) -> tuple[str, str] | None: + """(changed module file, attribute) that a string "pkg.mod.name" points at, if any.""" + parts = dotted.split(".") + changed = set(changed) + for cut in range(len(parts) - 1, 0, -1): + for module_file in _module_files(".".join(parts[:cut])): + if module_file in changed: + return module_file, parts[cut] + return None + + +def _binding_pattern(name: str) -> re.Pattern[str]: + """Lines that bind ``name`` in a module: import it, define it, or assign it. + + A name the diff merely mentions (``typer.confirm`` where ``typer`` was + imported long ago) is not one a patch target needs from the diff. + """ + n = re.escape(name) + return re.compile( + rf"^\s*(?:from\s+\S+\s+)?import\b.*\b{n}\b" # import name / from x import name + rf"|^\s*(?:async\s+)?(?:def|class)\s+{n}\b" # def name / class name + rf"|^\s*{n}\s*(?::[^=]*)?=(?!=)" # name = ... / name: T = ... + rf"|^\s*{n}\s*,?\s*(?:#.*)?$" # a line of a parenthesized import list + ) + + +def _patch_target_needs( + groups: list[Group], parsed_diff: ParsedDiff, by_file: dict[str, list[str]] +) -> Iterable[Need]: + """A string "pkg.mod.name" (a mock.patch target) needs the group that adds ``name``. + + Patching an attribute the module does not have yet fails, and the name + hides in a string, where symbol analysis does not look for it. + """ + added = added_lines_by_file(groups, parsed_diff) + for group in groups: + for lines in added.get(group.id, {}).values(): + for dotted in {d for line in lines for d in _DOTTED_STRING.findall(line)}: + target = _patch_target(dotted, by_file) + if target is None: + continue + module_file, name = target + binds = _binding_pattern(name) + for provider in by_file[module_file]: + provided = added.get(provider, {}).get(module_file, []) + if provider != group.id and any(binds.search(line) for line in provided): + reason = f"{group.id} patches {dotted}, which {provider} adds" + yield Need(group.id, provider, reason) + + +def _test_import_needs( + groups: list[Group], + by_file: dict[str, list[str]], + imports: _ImportGraph, +) -> Iterable[Need]: + """A test-only group needs every changed module its tests import, and their conftests.""" + changed_sources = {path for path in by_file if not is_test_path(path)} + changed_conftests = [p for p in by_file if PurePosixPath(p).name == "conftest.py"] + for group in groups: + # A group of code plus the test updates that follow it (see + # _attach_test_updates) keeps only the needs its lines name: the + # whole import reach of a test file, or its conftest, would tie it + # to every change the tested code sees. + if not all(is_test_path(a.file_path) for a in group.assignments): + continue + for assignment in group.assignments: + path = assignment.file_path + providers: dict[str, str] = {} + if path.endswith(".py"): + for target in sorted(imports.reachable(path) & changed_sources): + for provider in by_file[target]: + providers.setdefault(provider, target) + else: + # No import analysis for this language: pair test_x/x_test with x. + stem = PurePosixPath(path).stem.removeprefix("test_").removesuffix("_test") + for target in sorted(changed_sources): + if PurePosixPath(target).stem == stem: + for provider in by_file[target]: + providers.setdefault(provider, target) + # A test that loads a file by path (a script, a data file) names it + # in a string rather than importing it. + source_text = imports.text(path) + for target in sorted(changed_sources): + quoted = rf"""["'/]{re.escape(PurePosixPath(target).name)}["']""" + if re.search(quoted, source_text): + for provider in by_file[target]: + providers.setdefault(provider, target) + test_dir = PurePosixPath(path).parent + for conftest in changed_conftests: + conftest_dir = PurePosixPath(conftest).parent + if conftest != path and ( + conftest_dir == test_dir or conftest_dir in test_dir.parents + ): + for provider in by_file[conftest]: + providers.setdefault(provider, conftest) + for provider, target in providers.items(): + if provider != group.id: + yield Need(group.id, provider, f"{group.id} tests {target}, in {provider}") + + +_DEF_LINE = re.compile(r"^\s*(?:async\s+)?(?:def|class)\s|^\S") + + +def _same_definition(above: list[str], gap: list[str]) -> bool: + """Whether the unchanged ``gap`` lines leave the definition ``above`` ends in. + + They do at a new def/class or a top-level line, except the def that a + decorator stack ``above`` ends in decorates: that is still the same one. + """ + significant = [line for line in gap if line.strip()] + last_above = next((line for line in reversed(above) if line.strip()), "") + in_decorators = last_above.lstrip().startswith("@") + for line in significant: + if in_decorators and line.lstrip().startswith("@"): + continue + if in_decorators and re.match(r"^\s*(?:async\s+)?def\s", line): + in_decorators = False # the decorated def itself + continue + if _DEF_LINE.match(line): + return False + return True + + +def _change_span(hunk: Hunk) -> tuple[int, int]: + """0-based [first, last) target-line range the hunk's added/removed lines cover.""" + position = hunk.target_start - 1 + first: int | None = None + last = position + for line in hunk: + if line.is_added or line.is_removed: + if first is None: + first = position + last = position + (1 if line.is_added else 0) + if not line.is_removed: + position += 1 + return (first if first is not None else position), last + + +def hunk_ties( + parsed_diff: ParsedDiff, read_file: Callable[[str], str | None] | None +) -> dict[str, list[list[int]]]: + """Per file, clusters of hunks that edit the same function or class. + + Git splits one edit into two hunks when enough unchanged lines separate + them: a decorator added above a long signature and the parameter it + injects, say. Apart, neither half works. Two neighbouring hunks are tied + when no line between them starts a new definition (or leaves the + indented block), read from the file at the dev branch's head. + """ + ties: dict[str, list[list[int]]] = {} + for patch_file in parsed_diff.patch_set: + if len(patch_file) < 2 or read_file is None or patch_file.is_removed_file: + continue + text = read_file(patch_file.path) + if text is None: + continue + lines = text.splitlines() + clusters: list[list[int]] = [[0]] + for idx in range(1, len(patch_file)): + # Unchanged lines from the end of one edit to the start of the next, + # including each hunk's own context around the edit. + end = _change_span(patch_file[idx - 1])[1] + start = _change_span(patch_file[idx])[0] + gap = lines[end:start] + if start > end and _same_definition(lines[:end], gap): + clusters[-1].append(idx) + else: + clusters.append([idx]) + tied = [c for c in clusters if len(c) > 1] + if tied: + ties[patch_file.path] = tied + return ties + + +def _tie_needs( + groups: list[Group], ties: dict[str, list[list[int]]], parsed_diff: ParsedDiff +) -> Iterable[Need]: + """Groups holding hunks of one function need each other: they must be one group.""" + counts = {pf.path: len(pf) for pf in parsed_diff.patch_set} + holder: dict[tuple[str, int], str] = {} + for group in groups: + for assignment in group.assignments: + for idx in assignment.covered_indices(counts.get(assignment.file_path, 0)): + holder[(assignment.file_path, idx)] = group.id + for path, clusters in ties.items(): + for cluster in clusters: + owners = sorted({holder[(path, i)] for i in cluster if (path, i) in holder}) + for a, b in itertools.pairwise(owners): + reason = f"{a} and {b} edit the same definition in {path}" + yield Need(a, b, reason) + yield Need(b, a, reason) + + +def _lockfile_needs(by_file: dict[str, list[str]]) -> Iterable[Need]: + """A lockfile and its manifest ship together: each needs the other.""" + for lock_path, lock_groups in by_file.items(): + lock = PurePosixPath(lock_path) + for manifest_name in LOCKFILE_MANIFESTS.get(lock.name, ()): + manifest_path = str(lock.with_name(manifest_name)) + for lock_group in lock_groups: + for manifest_group in by_file.get(manifest_path, []): + if lock_group == manifest_group: + continue + reason = f"{lock_path} must ship with {manifest_path}" + yield Need(lock_group, manifest_group, reason) + yield Need(manifest_group, lock_group, reason) + + +def find_needs( + groups: list[Group], + parsed_diff: ParsedDiff, + read_file: Callable[[str], str | None] | None = None, + ties: dict[str, list[list[int]]] | None = None, +) -> list[Need]: + by_file = _groups_by_file(groups) + imports = _ImportGraph(parsed_diff, read_file) + needs: dict[tuple[str, str], Need] = {} + for need in ( + *_tie_needs(groups, ties or {}, parsed_diff), + *_symbol_needs(groups, parsed_diff), + *_patch_target_needs(groups, parsed_diff, by_file), + *_test_import_needs(groups, by_file, imports), + *_lockfile_needs(by_file), + ): + needs.setdefault((need.user, need.provider), need) + return list(needs.values()) + + +def _strongly_connected(nodes: list[str], edges: dict[str, set[str]]) -> list[list[str]]: + """Tarjan's algorithm, iterative so a long chain cannot exhaust the stack.""" + index: dict[str, int] = {} + low: dict[str, int] = {} + on_stack: set[str] = set() + stack: list[str] = [] + components: list[list[str]] = [] + counter = 0 + for root in nodes: + if root in index: + continue + work = [(root, iter(sorted(edges.get(root, ()))))] + index[root] = low[root] = counter + counter += 1 + stack.append(root) + on_stack.add(root) + while work: + node, children = work[-1] + child = next(children, None) + if child is not None: + if child not in index: + index[child] = low[child] = counter + counter += 1 + stack.append(child) + on_stack.add(child) + work.append((child, iter(sorted(edges.get(child, ()))))) + elif child in on_stack: + low[node] = min(low[node], index[child]) + continue + work.pop() + if work: + low[work[-1][0]] = min(low[work[-1][0]], low[node]) + if low[node] == index[node]: + component: list[str] = [] + while True: + member = stack.pop() + on_stack.discard(member) + component.append(member) + if member == node: + break + components.append(component) + return components + + +def _combine(keep: Group, absorbed: list[Group], hunk_counts: dict[str, int]) -> None: + hunks: dict[str, set[int]] = defaultdict(set) + for group in (keep, *absorbed): + for assignment in group.assignments: + count = hunk_counts.get(assignment.file_path, 0) + hunks[assignment.file_path].update(assignment.covered_indices(count)) + keep.assignments = [ + GroupAssignment( + file_path=path, + assignment_type=( + AssignmentType.WHOLE_FILE + if sorted(indices) == list(range(hunk_counts.get(path, 0))) + else AssignmentType.PARTIAL_HUNKS + ), + hunk_indices=sorted(indices), + ) + for path, indices in sorted(hunks.items()) + ] + descriptions = [g.description for g in (keep, *absorbed) if g.description] + keep.description = "\n\n".join(dict.fromkeys(descriptions)) + + +def _condense( + groups: list[Group], + edges: dict[str, set[str]], + needs: list[Need], + parsed_diff: ParsedDiff, +) -> tuple[list[Group], dict[str, set[str]]]: + """Combine every set of groups that need each other into one group.""" + order = [g.id for g in groups] + by_id = {g.id: g for g in groups} + position = {gid: i for i, gid in enumerate(order)} + merged_into: dict[str, str] = {} + hunk_counts = {pf.path: len(pf) for pf in parsed_diff.patch_set} + for component in _strongly_connected(order, edges): + if len(component) < 2: + continue + members = sorted(component, key=position.__getitem__) + # Keep the largest group's id and title: it is most of what the PR holds. + keep_id = max(members, key=lambda gid: (by_id[gid].estimated_loc, -position[gid])) + others = [gid for gid in members if gid != keep_id] + member_set = set(members) + reasons = [n.reason for n in needs if n.user in member_set and n.provider in member_set] + logger.info( + logs.GROUPS_COMBINED.format( + keep=keep_id, + others=", ".join(others), + reasons="; ".join(reasons[:3]) or "a test update and the code it adapts to", + ) + ) + _combine(by_id[keep_id], [by_id[gid] for gid in others], hunk_counts) + for gid in others: + merged_into[gid] = keep_id + + kept = [g for g in groups if g.id not in merged_into] + condensed: dict[str, set[str]] = {} + for group in kept: + members = {group.id} | {gid for gid, keep in merged_into.items() if keep == group.id} + deps: set[str] = set() + for member in members: + deps.update(merged_into.get(dep, dep) for dep in edges[member]) + condensed[group.id] = deps - {group.id} + recompute_estimated_loc(kept, parsed_diff) + return kept, condensed + + +def _words(lines: Iterable[str]) -> set[str]: + return { + word + for line in lines + for word in _IDENTIFIER.findall(line) + if len(word) >= _MIN_WORD_LENGTH and word not in _COMMON_WORDS + } + + +def _attach_test_updates( + groups: list[Group], + edges: dict[str, set[str]], + parsed_diff: ParsedDiff, + ties: dict[str, list[list[int]]], + direct_files: Callable[[str], set[str]], +) -> tuple[list[Group], dict[str, set[str]]]: + """Move each changed existing test into the group whose code change it follows. + + A hunk that rewrites or removes existing test lines is usually adapting + a test to new behaviour; left in a later test-only PR, the old test + fails in the PR that changes the behaviour. It moves to the group whose + changed source lines share the most names with it (the functions it + calls, the messages it checks). The caller reads needs again afterwards, + so the moved hunk's own needs (the names it uses) come along. + """ + files = {pf.path: pf for pf in parsed_diff.patch_set} + by_id = {g.id: g for g in groups} + changed_words: dict[str, set[str]] = {} + for group in groups: + lines = [ + line.value + for a in group.assignments + if not is_test_path(a.file_path) and a.file_path in files + for idx in a.covered_indices(len(files[a.file_path])) + for line in files[a.file_path][idx] + ] + # Context lines count too: a changed body sits under the unchanged + # def line that names it, which is what a test of it mentions. + changed_words[group.id] = _words(lines) + + moved = 0 + for test_group in groups: + for assignment in list(test_group.assignments): + patch_file = files.get(assignment.file_path) + if patch_file is None or not is_test_path(assignment.file_path): + continue + held = set(assignment.covered_indices(len(patch_file))) + cluster_of = {i: c for c in ties.get(assignment.file_path, []) for i in c} + done: set[int] = set() + for idx in sorted(held): + if idx in done: + continue + # Hunks of one test function move together (see hunk_ties). + cluster = [i for i in cluster_of.get(idx, [idx]) if i in held] + done.update(cluster) + hunks = [patch_file[i] for i in cluster] + # A conftest change or an autouse fixture stubs out or sets up + # what new code needs, so the code's own existing tests need it + # even when it only adds lines. + support = PurePosixPath(assignment.file_path).name == "conftest.py" or any( + "autouse" in line.value for hunk in hunks for line in hunk if line.is_added + ) + if patch_file.is_added_file or not ( + support or any(map(_changes_existing_tests, hunks)) + ): + continue # new tests only: they can come after the code + words = _words(line.value for hunk in hunks for line in hunk) + direct = direct_files(assignment.file_path) + # What the hunk's new lines patch ("pkg.mod.name") is the change it + # adapts to; failing that, prefer a group holding a file the + # test imports or names over one it reaches through others. + patched = { + target[0] + for hunk in hunks + for line in hunk + if line.is_added + for dotted in _DOTTED_STRING.findall(line.value) + if (target := _patch_target(dotted, files)) is not None + } + candidates = [ + ( + any(a.file_path in patched for a in by_id[gid].assignments), + any(a.file_path in direct for a in by_id[gid].assignments), + len(words & changed_words[gid]), + gid, + ) + for gid in edges.get(test_group.id, set()) | {test_group.id} + if changed_words.get(gid) + ] + if not candidates: + continue + *_, score, target = max(candidates) + if score == 0 or target == test_group.id: + continue + for i in cluster: + _move_hunk(test_group, by_id[target], assignment.file_path, i, len(patch_file)) + moved += len(cluster) + if moved: + logger.info(logs.TEST_UPDATES_ATTACHED.format(count=moved)) + kept = [g for g in groups if g.assignments] + kept_ids = {g.id for g in kept} + for gid in list(edges): + if gid not in kept_ids: + # An emptied group's dependents inherit what it depended on. + for other in kept_ids: + if gid in edges[other]: + edges[other] |= edges[gid] - {other} + del edges[gid] + for gid in kept_ids: + edges[gid] &= kept_ids + recompute_estimated_loc(kept, parsed_diff) + return kept, edges + + +_NEW_BLOCK = re.compile(r"^\s*(?:@|(?:async\s+)?def\s|class\s|$)") + + +def _changes_existing_tests(hunk: Hunk) -> bool: + """Whether a test hunk edits existing tests rather than only adding new ones. + + Removed lines edit something. So do added lines that land inside an + existing block, such as a new expected argument in an assertion: the + first added line then continues indented code instead of opening a new + function, class or decorator. + """ + if hunk.removed: + return True + added = [line.value.rstrip("\n") for line in hunk if line.is_added] + first = next((line for line in added if line.strip()), "") + return bool(first) and first[:1].isspace() and not _NEW_BLOCK.match(first) + + +def _ancestors(edges: dict[str, set[str]], gid: str) -> set[str]: + seen: set[str] = set() + pending = list(edges.get(gid, ())) + while pending: + dep = pending.pop() + if dep not in seen: + seen.add(dep) + pending.extend(edges.get(dep, ())) + return seen + + +def _carry_imports( + groups: list[Group], edges: dict[str, set[str]], parsed_diff: ParsedDiff +) -> None: + """Move a test file's added imports to the groups that now use them. + + Test updates moved next to their code (``_attach_test_updates``) still + use names the file's import block binds, and that block stays with the + remaining tests, which run after all the code: each moved update would + need a group that needs it back. Each import hunk moves instead to the + group using it that the other users already build on (or the one with + the fewest dependencies); the remaining tests then build on that group. + """ + files = {pf.path: pf for pf in parsed_diff.patch_set} + holder: dict[tuple[str, int], Group] = {} + for group in groups: + for a in group.assignments: + if a.file_path in files: + for idx in a.covered_indices(len(files[a.file_path])): + holder[(a.file_path, idx)] = group + for path, patch_file in files.items(): + if not is_test_path(path) or patch_file.is_added_file: + continue + for idx, hunk in enumerate(patch_file): + owner = holder.get((path, idx)) + if owner is None or not all(is_test_path(a.file_path) for a in owner.assignments): + continue + bound = import_bindings([line.value.rstrip("\n") for line in hunk if line.is_added]) + if not bound: + continue + users = [] + for other_idx, other in enumerate(patch_file): + user = holder.get((path, other_idx)) + if user is None or user is owner or user in users: + continue + if _words(line.value for line in other if line.is_added) & bound: + users.append(user) + if not users: + continue + ancestry = {u.id: _ancestors(edges, u.id) for u in users} + target = next( + (u for u in users if all(u.id in ancestry[o.id] or o is u for o in users)), + min(users, key=lambda u: len(ancestry[u.id])), + ) + _move_hunk(owner, target, path, idx, len(patch_file)) + holder[(path, idx)] = target + edges[owner.id].add(target.id) + + +def _move_hunk(source: Group, target: Group, path: str, idx: int, hunk_count: int) -> None: + for assignment in list(source.assignments): + if assignment.file_path != path: + continue + remaining = [i for i in assignment.covered_indices(hunk_count) if i != idx] + if remaining: + assignment.assignment_type = AssignmentType.PARTIAL_HUNKS + assignment.hunk_indices = remaining + else: + source.assignments.remove(assignment) + for assignment in target.assignments: + if assignment.file_path == path: + indices = sorted(set(assignment.covered_indices(hunk_count)) | {idx}) + assignment.hunk_indices = indices + assignment.assignment_type = ( + AssignmentType.WHOLE_FILE + if indices == list(range(hunk_count)) + else AssignmentType.PARTIAL_HUNKS + ) + return + target.assignments.append( + GroupAssignment( + file_path=path, + assignment_type=( + AssignmentType.WHOLE_FILE if hunk_count == 1 else AssignmentType.PARTIAL_HUNKS + ), + hunk_indices=[idx], + ) + ) + + +def _peel_tests(groups: list[Group], keep_declared_deps: bool) -> list[Group]: + """Split each group that mixes source and test files into a source and a test group. + + A test that sits in an unrelated source group ties that group to the code + it tests; a few such ties make every group need every other, and fixing + that by combining would leave one giant PR. Tests are placed again once + the source groups are in order (see ``_rehome_tests``). + """ + numbered = all(re.fullmatch(r"pr-\d+", g.id) for g in groups) + next_number = max((int(g.id[3:]) for g in groups), default=0) + 1 if numbered else 0 + peeled: list[Group] = [] + for group in groups: + tests = [a for a in group.assignments if is_test_path(a.file_path)] + if not tests or len(tests) == len(group.assignments): + peeled.append(group) + continue + group.assignments = [a for a in group.assignments if not is_test_path(a.file_path)] + if numbered: + test_id = f"pr-{next_number}" + next_number += 1 + else: + test_id = f"{group.id}-tests" + test_group = group.model_copy( + update={ + "id": test_id, + "title": f"Tests for {group.title}", + "assignments": tests, + "depends_on": list(group.depends_on) if keep_declared_deps else [], + } + ) + peeled.extend([group, test_group]) + return peeled + + +def _is_acyclic(edges: dict[str, set[str]]) -> bool: + try: + tuple(TopologicalSorter(edges).static_order()) + except CycleError: + return False + return True + + +def _fold(edges: dict[str, set[str]], absorbed: str, keep: str) -> dict[str, set[str]]: + """The dependency edges once ``absorbed`` is part of ``keep``.""" + folded: dict[str, set[str]] = {} + for gid, deps in edges.items(): + if gid == absorbed: + continue + mapped = {keep if d == absorbed else d for d in deps} + if gid == keep: + mapped |= {keep if d == absorbed else d for d in edges[absorbed]} + folded[gid] = mapped - {gid} + return folded + + +def _rehome_tests( + groups: list[Group], + edges: dict[str, set[str]], + parsed_diff: ParsedDiff, + max_loc: int | None, +) -> tuple[list[Group], dict[str, set[str]]]: + """Put each test-only group back with the code it tests, where that fits. + + A test group joins the one group it needs that no other group it needs + builds on (so the combined PR still comes after all of them), if the + result stays within ``max_loc``. Otherwise it stays a PR of its own, + ordered after the code it tests. + """ + by_id = {g.id: g for g in groups} + hunk_counts = {pf.path: len(pf) for pf in parsed_diff.patch_set} + for test_group in list(groups): + if not all(is_test_path(a.file_path) for a in test_group.assignments): + continue + providers = edges.get(test_group.id, set()) + test_stems = { + PurePosixPath(a.file_path).stem.removeprefix("test_").removesuffix("_test") + for a in test_group.assignments + } + + def preference(gid: str, stems: set[str] = test_stems) -> tuple[bool, int]: + files = {PurePosixPath(a.file_path).stem for a in by_id[gid].assignments} + return (not files & stems, by_id[gid].estimated_loc) + + for candidate in sorted(providers, key=preference): + size = by_id[candidate].estimated_loc + test_group.estimated_loc + if max_loc is not None and size > max_loc: + continue + folded = _fold(edges, test_group.id, candidate) + if not _is_acyclic(folded): + continue + _combine(by_id[candidate], [test_group], hunk_counts) + by_id[candidate].estimated_loc = size + edges = folded + groups = [g for g in groups if g.id != test_group.id] + break + return groups, edges + + +def make_groups_standalone( + groups: list[Group], + parsed_diff: ParsedDiff, + read_file: Callable[[str], str | None] | None = None, + max_loc: int | None = None, + *, + keep_declared_deps: bool = True, +) -> list[Group]: + """Give every group the dependencies it needs, so each sub-PR works on its own. + + Tests are first separated from unrelated code, then each group gets a + dependency on every group whose code it needs; groups that need each + other are combined, and each test goes back to the group whose code it + tests when that fits within ``max_loc``. ``keep_declared_deps`` keeps the + dependencies a planner chose (an LLM's); without it, the only declared + edges are file order, which is derived again after tests are separated. + """ + if len(groups) < 2: + return groups + if read_file is not None: + # Every pass below reads the same files; git show them once. + read_file = functools.cache(read_file) + groups = _peel_tests(groups, keep_declared_deps) + if not keep_declared_deps: + # A generated plan's edges only encode file order, and peeling moved + # files between groups; derive them again from the new layout. + for group in groups: + group.depends_on = [] + derive_file_order_dependencies(groups) + ties = hunk_ties(parsed_diff, read_file) + needs = find_needs(groups, parsed_diff, read_file, ties) + edges: dict[str, set[str]] = {g.id: set(g.depends_on) for g in groups} + for need in needs: + edges[need.user].add(need.provider) + recompute_estimated_loc(groups, parsed_diff) + kept, condensed = _condense(groups, edges, needs, parsed_diff) + imports = _ImportGraph(parsed_diff, read_file) + kept, condensed = _attach_test_updates( + kept, condensed, parsed_diff, ties, imports.direct_targets + ) + _carry_imports(kept, condensed, parsed_diff) + # A moved hunk brings needs its new group did not have (an import line + # left behind in its old group, say); read them again from the new layout. + needs = find_needs(kept, parsed_diff, read_file, ties) + for need in needs: + condensed[need.user].add(need.provider) + kept, condensed = _condense(kept, condensed, needs, parsed_diff) + kept, condensed = _rehome_tests(kept, condensed, parsed_diff, max_loc) + for group in kept: + group.depends_on = sorted(condensed[group.id]) + reduce_transitive_dependencies(kept) + recompute_estimated_loc(kept, parsed_diff) + return kept diff --git a/pr_split/planner/partitioning.py b/pr_split/planner/partitioning.py index 4248baa..3fdd974 100644 --- a/pr_split/planner/partitioning.py +++ b/pr_split/planner/partitioning.py @@ -1,6 +1,5 @@ from __future__ import annotations -import os from collections import defaultdict from dataclasses import dataclass from graphlib import CycleError, TopologicalSorter @@ -16,7 +15,7 @@ from ..graph import PlanDAG from ..schemas import Group, GroupAssignment from .chunker import recompute_estimated_loc -from .symbols import names_in, symbol_dependencies +from .symbols import LOCKFILES, is_test_path, names_in if TYPE_CHECKING: from collections.abc import Iterable, Sequence @@ -147,7 +146,7 @@ def _affinity_score(unit_a: PartitionUnit, unit_b: PartitionUnit, priority: Prio def _merge_order_is_acyclic(grouped_units: Iterable[Sequence[PartitionUnit]]) -> bool: """Check that the merge-order dependencies implied by ``grouped_units`` form a DAG. - Mirrors ``_derive_merge_order_dependencies``: within each file, groups are ordered by + Mirrors ``derive_file_order_dependencies``: within each file, groups are ordered by their earliest unit and each group depends on the one before it. """ file_occurrences: dict[str, list[tuple[int, int]]] = defaultdict(list) @@ -514,29 +513,76 @@ def _group_units_cp_sat( return grouped -def _build_group_title(group_index: int, units: list[PartitionUnit]) -> str: - file_paths = sorted({unit.file_path for unit in units}) - if len(file_paths) == 1: - stem = PurePosixPath(file_paths[0]).stem.replace("_", "-") - return f"chore(split): review {stem}-{group_index}" - # Name a multi-file group after the directory its files share, or after - # its largest file, so every group title says what it covers. - parents = {PurePosixPath(path).parent for path in file_paths} - common = PurePosixPath(os.path.commonpath([str(p) for p in parents])) if parents else None - if common is not None and str(common) not in ("", "."): - return f"chore(split): review {common.name}-{group_index}" - largest = max(units, key=lambda unit: unit.loc).file_path - stem = PurePosixPath(largest).stem.replace("_", "-") - return f"chore(split): review {stem} and {len(file_paths) - 1} more-{group_index}" +def _file_loc(parsed_diff: ParsedDiff) -> dict[str, dict[int, int]]: + return { + pf.path: {i: hunk.added + hunk.removed for i, hunk in enumerate(pf)} + for pf in parsed_diff.patch_set + } + + +def _plural(count: int, noun: str) -> str: + return f"{count} {noun}" if count == 1 else f"{count} {noun}s" -def _describe_files(backend: str, file_paths: Iterable[str]) -> str: +def retitle_groups(groups: list[Group], parsed_diff: ParsedDiff) -> None: + """Title each group after what it changes: its main file, and its part of it. + + A title names the verb (Add for new files, Remove for deleted ones, + Update otherwise), the group's largest non-test file, and how many other + files it touches, so a reviewer sees what the PR covers from its title. + """ + new_files = {pf.path for pf in parsed_diff.patch_set if pf.is_added_file} + deleted = {pf.path for pf in parsed_diff.patch_set if pf.is_removed_file} + hunk_loc = _file_loc(parsed_diff) + hunk_counts = {path: len(locs) for path, locs in hunk_loc.items()} + # Which groups hold a piece of each file, in hunk order, for "part k of n". + holders: dict[str, list[tuple[int, str]]] = defaultdict(list) + for group in groups: + for assignment in group.assignments: + indices = assignment.covered_indices(hunk_counts.get(assignment.file_path, 0)) + holders[assignment.file_path].append((min(indices, default=0), group.id)) + for group in groups: + loc: dict[str, int] = defaultdict(int) + for assignment in group.assignments: + locs = hunk_loc.get(assignment.file_path, {}) + for idx in assignment.covered_indices(len(locs)): + loc[assignment.file_path] += locs.get(idx, 0) + paths = sorted(loc) + if not paths: + continue + if all(p in new_files for p in paths): + verb = "Add" + elif all(p in deleted for p in paths): + verb = "Remove" + else: + verb = "Update" + # Name the group after code, not its tests or a lockfile it ships. + sources = ( + [p for p in paths if not is_test_path(p) and PurePosixPath(p).name not in LOCKFILES] + or [p for p in paths if not is_test_path(p)] + or paths + ) + main = max(sources, key=lambda p: (loc[p], p)) + subject = main + pieces = sorted(holders[main]) + if len(pieces) > 1: + part = [gid for _, gid in pieces].index(group.id) + 1 + subject += f" (part {part} of {len(pieces)})" + others = len(paths) - 1 + if others: + tests_only = all(is_test_path(p) for p in paths if p != main) + noun = "test file" if tests_only and not is_test_path(main) else "more file" + subject += f" and {_plural(others, noun)}" + group.title = f"{verb} {subject}" + + +def describe_files(backend: str, file_paths: Iterable[str]) -> str: paths = sorted(set(file_paths)) return f"{backend} partition over {len(paths)} file(s): {', '.join(paths)}" def _build_group_description(backend: PartitionStrategy, units: list[PartitionUnit]) -> str: - return _describe_files(backend.value, (unit.file_path for unit in units)) + return describe_files(backend.value, (unit.file_path for unit in units)) def refresh_generated_description(group: Group) -> None: @@ -552,7 +598,7 @@ def refresh_generated_description(group: Group) -> None: return if len(files.split(", ") if files else []) != int(count): return - group.description = _describe_files(backend, (a.file_path for a in group.assignments)) + group.description = describe_files(backend, (a.file_path for a in group.assignments)) def _build_groups_from_units( @@ -588,7 +634,7 @@ def _build_groups_from_units( groups.append( Group( id=f"pr-{group_index}", - title=_build_group_title(group_index, units), + title=f"pr-{group_index}", description=_build_group_description(backend, units), assignments=sorted(assignments, key=lambda assignment: assignment.file_path), estimated_loc=sum(unit.loc for unit in units), @@ -596,38 +642,12 @@ def _build_groups_from_units( ) recompute_estimated_loc(groups, parsed_diff) - _derive_merge_order_dependencies(groups) - _add_symbol_dependencies(groups, parsed_diff) + retitle_groups(groups, parsed_diff) + derive_file_order_dependencies(groups) return groups -def _add_symbol_dependencies(groups: list[Group], parsed_diff: ParsedDiff) -> None: - """Make a group depend on every group whose newly defined names it uses. - - File order alone misses a new test file that imports a new module, so the - test's sub-PR would target the base and fail on import. An edge that - would close a cycle is skipped with a warning. - """ - by_id = {g.id: g for g in groups} - for user, definers in sorted(symbol_dependencies(groups, parsed_diff).items()): - for definer, names in sorted(definers.items()): - group = by_id[user] - if definer in group.depends_on: - continue - group.depends_on.append(definer) - try: - PlanDAG(groups).validate_acyclic() - except PRSplitError: - group.depends_on.remove(definer) - logger.warning( - logs.SYMBOL_EDGE_SKIPPED.format( - user=user, definer=definer, names=", ".join(sorted(names)[:5]) - ) - ) - _reduce_transitive_dependencies(groups) - - -def _reduce_transitive_dependencies(groups: list[Group]) -> None: +def reduce_transitive_dependencies(groups: list[Group]) -> None: dep_map = {group.id: set(group.depends_on) for group in groups} for group in groups: reduced_deps = set(dep_map[group.id]) @@ -653,7 +673,7 @@ def _has_alternative_path(dep_map: dict[str, set[str]], source: str, target: str return False -def _derive_merge_order_dependencies(groups: list[Group]) -> None: +def derive_file_order_dependencies(groups: list[Group]) -> None: file_occurrences: dict[str, list[tuple[int, str]]] = defaultdict(list) for group in groups: for assignment in group.assignments: @@ -671,7 +691,7 @@ def _derive_merge_order_dependencies(groups: list[Group]) -> None: for group in groups: group.depends_on = sorted(dep_map[group.id]) - _reduce_transitive_dependencies(groups) + reduce_transitive_dependencies(groups) def partition_diff(parsed_diff: ParsedDiff, settings: Settings) -> list[Group]: diff --git a/pr_split/planner/symbols.py b/pr_split/planner/symbols.py index cb0b1e1..df82c2b 100644 --- a/pr_split/planner/symbols.py +++ b/pr_split/planner/symbols.py @@ -11,12 +11,19 @@ import re from collections import defaultdict +from pathlib import PurePosixPath from typing import TYPE_CHECKING if TYPE_CHECKING: from ..diff_ops import ParsedDiff from ..schemas import Group +# Class-level fields: dataclass, TypedDict, pydantic and attrs attributes +# (`` name: type`` one indent in). Code reads them as keys and attributes. A +# line ending in a comma is a parameter of a multi-line signature instead. +_FIELD_PATTERN = re.compile( + r"^(?: {4}|\t)([A-Za-z_]\w*)\s*:\s*[A-Za-z_][\w\[\], .|\"']*(?:=.*[^,\s])?\s*$" +) _DEFINITION_PATTERNS = ( # Python, Ruby: def/class, including async def. re.compile(r"^\s*(?:async\s+)?(?:def|class)\s+([A-Za-z_]\w*)"), @@ -29,19 +36,27 @@ r"^\s*(?:export\s+)?(?:pub(?:\([^)]*\))?\s+)?(?:abstract\s+|final\s+|sealed\s+|data\s+)*" r"(?:class|struct|enum|interface|trait|type|protocol|record)\s+([A-Za-z_]\w*)" ), - # Top-level constants: UPPER_CASE = ... (Python) or const/let/var NAME = (JS/TS). - re.compile(r"^([A-Z][A-Z0-9_]{2,})\s*(?::[^=]+)?=(?!=)"), + # Constants: UPPER_CASE = ... at top level or in a class body (an enum, a + # message catalogue), or const/let/var NAME = (JS/TS). + re.compile(r"^(?:(?: {4}|\t))?([A-Z][A-Z0-9_]{2,})\s*(?::[^=]+)?=(?!=)"), re.compile(r"^\s*(?:export\s+)?(?:const|let|var)\s+([A-Za-z_]\w*)\s*[:=]"), + _FIELD_PATTERN, ) +_FROM_IMPORT = re.compile(r"^\s*from\s+\S+\s+import\s+(.*)$") +_PLAIN_IMPORT = re.compile(r"^\s*import\s+(.*)$") +_ANNOTATED_NAME = re.compile(r"^\s+([A-Za-z_]\w*)\s*:") _IDENTIFIER = re.compile(r"[A-Za-z_]\w*") # Names too generic to tie two groups together. _MIN_NAME_LENGTH = 4 _IGNORED = frozenset({"main", "self", "this", "init", "test", "setup", "teardown", "None", "True"}) -def _added_lines_by_group(groups: list[Group], parsed_diff: ParsedDiff) -> dict[str, list[str]]: +def added_lines_by_file( + groups: list[Group], parsed_diff: ParsedDiff +) -> dict[str, dict[str, list[str]]]: + """group id -> file path -> the lines that group's hunks add to the file.""" files = {pf.path: pf for pf in parsed_diff.patch_set} - added: dict[str, list[str]] = defaultdict(list) + added: dict[str, dict[str, list[str]]] = defaultdict(lambda: defaultdict(list)) for group in groups: for assignment in group.assignments: patch_file = files.get(assignment.file_path) @@ -50,45 +65,206 @@ def _added_lines_by_group(groups: list[Group], parsed_diff: ParsedDiff) -> dict[ for idx in assignment.covered_indices(len(patch_file)): if idx >= len(patch_file): continue - added[group.id].extend( + added[group.id][assignment.file_path].extend( line.value.rstrip("\n") for line in patch_file[idx] if line.is_added ) return added +# Lockfiles, each resolved from the manifest(s) named here. +LOCKFILE_MANIFESTS = { + "uv.lock": ("pyproject.toml",), + "poetry.lock": ("pyproject.toml",), + "pdm.lock": ("pyproject.toml",), + "Pipfile.lock": ("Pipfile",), + "package-lock.json": ("package.json",), + "npm-shrinkwrap.json": ("package.json",), + "yarn.lock": ("package.json",), + "pnpm-lock.yaml": ("package.json",), + "bun.lockb": ("package.json",), + "Cargo.lock": ("Cargo.toml",), + "go.sum": ("go.mod",), + "Gemfile.lock": ("Gemfile",), + "composer.lock": ("composer.json",), + "mix.lock": ("mix.exs",), +} +LOCKFILES = frozenset(LOCKFILE_MANIFESTS) + +_TEST_DIRS = frozenset({"test", "tests", "__tests__", "spec", "specs", "testing"}) +_TEST_SUFFIXES = ("_test", "_tests", "_spec", ".test", ".spec", "Test", "Tests", "Spec") + + +def is_test_path(path: str) -> bool: + """Whether a path holds tests (or test fixtures such as conftest.py).""" + pure = PurePosixPath(path) + stem = pure.name.split(".", 1)[0] if pure.suffix else pure.name + full_stem = pure.name[: -len(pure.suffix)] if pure.suffix else pure.name + return ( + any(part in _TEST_DIRS for part in pure.parts[:-1]) + or stem.startswith("test_") + or stem == "conftest" + or full_stem.endswith(_TEST_SUFFIXES) + ) + + def _definitions(lines: list[str]) -> set[str]: names: set[str] = set() for line in lines: for pattern in _DEFINITION_PATTERNS: match = pattern.match(line) if match: - names.add(match.group(1)) + name = match.group(1) + # A field named like a common word (branch, paths) is mentioned + # all over a codebase; only a specific one ties code together. + if pattern is _FIELD_PATTERN and ( + ("_" not in name and len(name) < 8) or line.rstrip().endswith(("[", "(")) + ): + continue + names.add(name) return {n for n in names if len(n) >= _MIN_NAME_LENGTH and n not in _IGNORED} +def _bound_name(item: str) -> str | None: + """The name ``x`` or ``y as x`` binds, or None for noise such as ``(`` or ``*``.""" + parts = item.strip().strip("(),").split() + name = parts[-1] if parts else "" + return name.split(".")[0] if _IDENTIFIER.fullmatch(name.split(".")[0] or "-") else None + + +def import_bindings(lines: list[str]) -> set[str]: + """Names that the import statements among ``lines`` bind in their file.""" + bound: set[str] = set() + in_list = False + for line in lines: + if in_list: + items = line.split(")")[0] + bound.update(filter(None, (_bound_name(i) for i in items.split(",")))) + in_list = ")" not in line + continue + match = _FROM_IMPORT.match(line) or _PLAIN_IMPORT.match(line) + if not match: + continue + items = match.group(1).split("#")[0] + in_list = items.strip().startswith("(") and ")" not in items + bound.update(filter(None, (_bound_name(i) for i in items.split(",")))) + return {name for name in bound if name != "import"} + + +def _is_test_module(path: str) -> bool: + """A test file proper (test_x.py, x_test.go), not a shared fixture or helper module.""" + stem = PurePosixPath(path).stem + return stem.startswith("test_") or stem.endswith(("_test", ".test", ".spec", "_spec")) + + +def _can_import(user_path: str, def_path: str, user_is_test: bool) -> bool: + """Whether code in ``user_path`` can use a name that ``def_path`` defines. + + Non-test code never imports tests. A test file's own helpers stay in that + file (another test file defining a same-named helper is not using it); + only conftest files and shared helper modules serve other test files. + """ + if not is_test_path(def_path): + return True + if not user_is_test: + return False + return user_path == def_path or not _is_test_module(def_path) + + +# A keyword argument (``name=value``, no spaces, as PEP 8 writes it) or a +# dict key (``"name": value``): how a TypedDict or dataclass field is filled. +_FIELD_WRITE = re.compile(r"""[(,\s]([A-Za-z_]\w*)=(?!=)|["']([A-Za-z_]\w*)["']\s*:""") + + +def _new_fields(added: dict[str, dict[str, list[str]]]) -> set[str]: + return { + match.group(1) + for by_file in added.values() + for lines in by_file.values() + for line in lines + if (match := _FIELD_PATTERN.match(line)) + } + + +def _existing_definitions(parsed_diff: ParsedDiff) -> dict[str, set[str]]: + """Per file, names its removed lines define: edited, not new, definitions. + + A changed signature or field is rewritten as a removed and an added line; + the name existed before the diff, so nothing needs the group that edits it. + """ + existing: dict[str, set[str]] = {} + for pf in parsed_diff.patch_set: + removed = [line.value.rstrip("\n") for hunk in pf for line in hunk if line.is_removed] + # Also any ``name:`` a removed line starts with: a parameter or field + # in whatever form the old line had it. + existing[pf.path] = _definitions(removed) | { + m.group(1) for line in removed if (m := _ANNOTATED_NAME.match(line)) + } + return existing + + def symbol_dependencies( groups: list[Group], parsed_diff: ParsedDiff ) -> dict[str, dict[str, set[str]]]: """Map user group -> definer group -> the names it uses from that definer. Only names defined by exactly one group count: a name several groups - define (an overload, a common helper name) says nothing about order. + define (an overload, a common helper name) says nothing about order. A + name defined in a test file is never a dependency of non-test code, which + cannot import it; the match is only a same-named attribute or word. """ - added = _added_lines_by_group(groups, parsed_diff) - defined_by: dict[str, list[str]] = defaultdict(list) + added = added_lines_by_file(groups, parsed_diff) + existing = _existing_definitions(parsed_diff) + defined_by: dict[str, dict[str, str]] = defaultdict(dict) + for group in groups: + for path, lines in added.get(group.id, {}).items(): + for name in _definitions(lines) - existing.get(path, set()): + defined_by[name].setdefault(group.id, path) + owner = {name: next(iter(by.items())) for name, by in defined_by.items() if len(by) == 1} + # A new field is only useful once something fills it: readers also need + # every group that writes it (``name=`` or ``"name":``). + fields = {name for name in owner if name in _new_fields(added)} + writers: dict[str, set[str]] = defaultdict(set) + for group in groups: + for path, lines in added.get(group.id, {}).items(): + if is_test_path(path): + continue # a test building a fixture fills nothing code reads + for line in lines: + for match in _FIELD_WRITE.finditer(line): + name = match.group(1) or match.group(2) + if name in fields: + writers[name].add(group.id) + + # An added import binds a name for its own file only: another group's + # lines in that file that use the name need the group holding the import. + importer: dict[tuple[str, str], set[str]] = defaultdict(set) for group in groups: - for name in _definitions(added.get(group.id, [])): - defined_by[name].append(group.id) - owner = {name: gids[0] for name, gids in defined_by.items() if len(gids) == 1} + for path, lines in added.get(group.id, {}).items(): + for name in import_bindings(lines): + importer[(path, name)].add(group.id) uses: dict[str, dict[str, set[str]]] = defaultdict(lambda: defaultdict(set)) for group in groups: - own = _definitions(added.get(group.id, [])) - for line in added.get(group.id, []): - for name in _IDENTIFIER.findall(line): - definer = owner.get(name) - if definer is not None and definer != group.id and name not in own: - uses[group.id][definer].add(name) + by_file = added.get(group.id, {}) + for path, lines in by_file.items(): + local = import_bindings(lines) + for name in {n for line in lines for n in _IDENTIFIER.findall(line)} - local: + binders = importer.get((path, name), set()) + if len(binders) == 1 and group.id not in binders and len(name) >= 3: + uses[group.id][next(iter(binders))].add(name) + own = _definitions([line for lines in by_file.values() for line in lines]) + for path, lines in by_file.items(): + user_is_test = is_test_path(path) + for line in lines: + for name in _IDENTIFIER.findall(line): + found = owner.get(name) + if found is None or name in own: + continue + definer, def_path = found + if not _can_import(path, def_path, user_is_test): + continue + for provider in {definer} | writers.get(name, set()): + if provider != group.id: + uses[group.id][provider].add(name) return {user: dict(definers) for user, definers in uses.items()} diff --git a/tests/test_coherence.py b/tests/test_coherence.py new file mode 100644 index 0000000..7ac3bb9 --- /dev/null +++ b/tests/test_coherence.py @@ -0,0 +1,357 @@ +"""Each group of a plan must work on its own: see pr_split/planner/coherence.py.""" + +from __future__ import annotations + +from pr_split.constants import AssignmentType +from pr_split.diff_ops.parser import parse_diff +from pr_split.graph import PlanDAG +from pr_split.planner.coherence import find_needs, hunk_ties, make_groups_standalone +from pr_split.planner.symbols import symbol_dependencies +from pr_split.schemas import Group, GroupAssignment + + +def _new_file(path: str, lines: list[str]) -> str: + body = "".join(f"+{line}\n" for line in lines) + return ( + f"diff --git a/{path} b/{path}\nnew file mode 100644\n--- /dev/null\n+++ b/{path}\n" + f"@@ -0,0 +1,{len(lines)} @@\n{body}" + ) + + +def _group(gid: str, *files: str | tuple[str, list[int]], deps: list[str] | None = None) -> Group: + assignments = [] + for item in files: + if isinstance(item, tuple): + path, hunks = item + kind = AssignmentType.PARTIAL_HUNKS + else: + path, hunks, kind = item, [0], AssignmentType.WHOLE_FILE + assignments.append( + GroupAssignment(file_path=path, assignment_type=kind, hunk_indices=hunks) + ) + return Group( + id=gid, title=gid, description=gid, assignments=assignments, depends_on=deps or [] + ) + + +def _reader(files: dict[str, str]): + return files.get + + +def _files_of(group: Group) -> set[str]: + return {a.file_path for a in group.assignments} + + +class TestNeeds: + def test_patch_target_needs_the_group_that_binds_the_name(self) -> None: + diff = _new_file("pkg/cli.py", ["from .tools import require_tools_now"]) + _new_file( + "tests/conftest.py", ['patch("pkg.cli.require_tools_now")'] + ) + groups = [_group("pr-1", "pkg/cli.py"), _group("pr-2", "tests/conftest.py")] + needs = {(n.user, n.provider) for n in find_needs(groups, parse_diff(diff))} + assert ("pr-2", "pr-1") in needs + + def test_patch_target_on_a_name_the_diff_only_mentions_needs_nothing(self) -> None: + # ``logger`` existed before the diff; the new line only calls it. + diff = _new_file("pkg/cli.py", ["logger.info('hello there')"]) + _new_file( + "tests/test_x.py", ['patch("pkg.cli.logger")'] + ) + groups = [_group("pr-1", "pkg/cli.py"), _group("pr-2", "tests/test_x.py")] + assert find_needs(groups, parse_diff(diff)) == [] + + def test_a_test_group_needs_every_changed_module_it_imports(self) -> None: + diff = _new_file("pkg/engine.py", ["VALUE = 1"]) + _new_file( + "tests/test_engine.py", ["def test_it():", " assert run_it()"] + ) + # The import sits outside the diff; only the file at head shows it. + head = { + "tests/test_engine.py": "from pkg.engine import run_it\n\ndef test_it():\n pass\n", + "pkg/engine.py": "VALUE = 1\n", + } + groups = [_group("pr-1", "pkg/engine.py"), _group("pr-2", "tests/test_engine.py")] + needs = find_needs(groups, parse_diff(diff), _reader(head)) + assert [(n.user, n.provider) for n in needs] == [("pr-2", "pr-1")] + + def test_imports_are_followed_through_unchanged_modules(self) -> None: + diff = _new_file("pkg/core.py", ["VALUE = 1"]) + _new_file( + "tests/test_api.py", ["def test_it():", " assert True"] + ) + head = { + "tests/test_api.py": "from pkg import api\n", + "pkg/__init__.py": "", + "pkg/api.py": "from .core import VALUE\n", + "pkg/core.py": "VALUE = 1\n", + } + groups = [_group("pr-1", "pkg/core.py"), _group("pr-2", "tests/test_api.py")] + needs = find_needs(groups, parse_diff(diff), _reader(head)) + assert [(n.user, n.provider) for n in needs] == [("pr-2", "pr-1")] + + def test_tests_need_the_conftest_above_them(self) -> None: + diff = _new_file("tests/conftest.py", ["import pytest"]) + _new_file( + "tests/unit/test_a.py", ["def test_a():", " pass"] + ) + groups = [_group("pr-1", "tests/conftest.py"), _group("pr-2", "tests/unit/test_a.py")] + needs = {(n.user, n.provider) for n in find_needs(groups, parse_diff(diff))} + assert ("pr-2", "pr-1") in needs + + def test_a_script_a_test_loads_by_path_is_a_need(self) -> None: + diff = _new_file("scripts/score.py", ["LIMIT = 3"]) + _new_file( + "tests/test_score.py", ['SCRIPT = ROOT / "scripts" / "score.py"'] + ) + groups = [_group("pr-1", "scripts/score.py"), _group("pr-2", "tests/test_score.py")] + needs = {(n.user, n.provider) for n in find_needs(groups, parse_diff(diff))} + assert ("pr-2", "pr-1") in needs + + +class TestSymbols: + def test_an_import_line_binds_the_name_for_its_own_file(self) -> None: + # The import and the test using it landed in different groups. + diff = """\ +diff --git a/tests/test_x.py b/tests/test_x.py +--- a/tests/test_x.py ++++ b/tests/test_x.py +@@ -1,2 +1,3 @@ + import os ++from pkg.mod import make_widget + +@@ -30,1 +31,3 @@ def test_old(): + pass ++def test_new(): ++ assert make_widget() +""" + groups = [ + _group("pr-1", ("tests/test_x.py", [0])), + _group("pr-2", ("tests/test_x.py", [1])), + ] + assert symbol_dependencies(groups, parse_diff(diff)) == {"pr-2": {"pr-1": {"make_widget"}}} + + def test_a_helper_local_to_one_test_file_is_not_shared(self) -> None: + diff = _new_file("tests/test_a.py", ["def _make_group():", " return 1"]) + _new_file( + "tests/test_b.py", ["def _make_group():", " return 2", "x = _make_group()"] + ) + groups = [_group("pr-1", "tests/test_a.py"), _group("pr-2", "tests/test_b.py")] + assert symbol_dependencies(groups, parse_diff(diff)) == {} + + def test_code_never_needs_a_name_a_test_defines(self) -> None: + diff = _new_file("tests/test_a.py", ["def capture_output():", " pass"]) + _new_file( + "pkg/cli.py", ["console.capture_output()"] + ) + groups = [_group("pr-1", "tests/test_a.py"), _group("pr-2", "pkg/cli.py")] + assert symbol_dependencies(groups, parse_diff(diff)) == {} + + def test_an_edited_signature_is_not_a_new_definition(self) -> None: + diff = """\ +diff --git a/pkg/a.py b/pkg/a.py +--- a/pkg/a.py ++++ b/pkg/a.py +@@ -1,2 +1,2 @@ +-def compute_total(x): ++def compute_total(x, y=0): + return x +diff --git a/pkg/b.py b/pkg/b.py +new file mode 100644 +--- /dev/null ++++ b/pkg/b.py +@@ -0,0 +1 @@ ++VALUE = compute_total(1) +""" + groups = [_group("pr-1", "pkg/a.py"), _group("pr-2", "pkg/b.py")] + assert symbol_dependencies(groups, parse_diff(diff)) == {} + + def test_reading_a_new_field_needs_the_code_that_fills_it(self) -> None: + diff = ( + _new_file("pkg/types.py", ["class Summary(TypedDict):", " hunk_indices: list[int]"]) + + _new_file("pkg/parser.py", ["summary = Summary(hunk_indices=[0])"]) + + _new_file("pkg/prompts.py", ['rows = summary["hunk_indices"]']) + ) + groups = [ + _group("pr-1", "pkg/types.py"), + _group("pr-2", "pkg/parser.py"), + _group("pr-3", "pkg/prompts.py"), + ] + deps = symbol_dependencies(groups, parse_diff(diff)) + assert set(deps["pr-3"]) == {"pr-1", "pr-2"} + + +class TestHunkTies: + DIFF = """\ +diff --git a/tests/test_x.py b/tests/test_x.py +--- a/tests/test_x.py ++++ b/tests/test_x.py +@@ -1,3 +1,4 @@ + class TestThing: ++ @patch("pkg.forget") + @patch("pkg.one") + @patch("pkg.two") +@@ -9,3 +10,4 @@ class TestThing: + mock_two, + mock_one, ++ mock_forget, + ) -> None: +""" + HEAD = ( + "class TestThing:\n" + ' @patch("pkg.forget")\n' + ' @patch("pkg.one")\n' + ' @patch("pkg.two")\n' + ' @patch("pkg.three")\n' + ' @patch("pkg.four")\n' + " def test_it(\n" + " self,\n" + " mock_four,\n" + " mock_three,\n" + " mock_two,\n" + " mock_one,\n" + " mock_forget,\n" + " ) -> None:\n" + ) + + def test_a_decorator_and_the_parameter_it_injects_are_tied(self) -> None: + ties = hunk_ties(parse_diff(self.DIFF), _reader({"tests/test_x.py": self.HEAD})) + assert ties == {"tests/test_x.py": [[0, 1]]} + + def test_hunks_in_different_functions_are_not_tied(self) -> None: + diff = """\ +diff --git a/pkg/a.py b/pkg/a.py +--- a/pkg/a.py ++++ b/pkg/a.py +@@ -1,2 +1,3 @@ + def first(): ++ x = 1 + return 1 +@@ -9,2 +10,3 @@ def second(): + def second(): ++ y = 2 + return 2 +""" + head = ( + "def first():\n x = 1\n return 1\n" + + "\n" * 6 + + ("def second():\n y = 2\n return 2\n") + ) + assert hunk_ties(parse_diff(diff), _reader({"pkg/a.py": head})) == {} + + def test_groups_holding_tied_hunks_become_one(self) -> None: + groups = [ + _group("pr-1", ("tests/test_x.py", [0])), + _group("pr-2", ("tests/test_x.py", [1])), + ] + kept = make_groups_standalone( + groups, parse_diff(self.DIFF), _reader({"tests/test_x.py": self.HEAD}) + ) + assert len(kept) == 1 + assert kept[0].assignments[0].hunk_indices == [0, 1] + + +class TestMakeGroupsStandalone: + def test_a_lockfile_ships_with_its_manifest(self) -> None: + diff = _new_file("uv.lock", ["version = 1"]) + _new_file("pyproject.toml", ["[project]"]) + groups = [_group("pr-1", "uv.lock"), _group("pr-2", "pyproject.toml")] + kept = make_groups_standalone(groups, parse_diff(diff), max_loc=1) + assert len(kept) == 1 + assert _files_of(kept[0]) == {"uv.lock", "pyproject.toml"} + + def test_tests_mixed_into_unrelated_code_are_separated(self) -> None: + # pr-1 packs a test for pr-2's module next to its own code; kept + # together, pr-1 would need pr-2 and pr-2 (file order) would need pr-1. + diff = ( + _new_file("pkg/alpha.py", ["ALPHA_VALUE = 1"]) + + _new_file("pkg/beta.py", ["def beta_value():", " return ALPHA_VALUE"]) + + _new_file("tests/test_beta.py", ["from pkg.beta import beta_value"]) + ) + groups = [ + _group("pr-1", "pkg/alpha.py", "tests/test_beta.py"), + _group("pr-2", "pkg/beta.py"), + ] + kept = make_groups_standalone( + groups, parse_diff(diff), max_loc=3, keep_declared_deps=False + ) + owner = {path: g.id for g in kept for path in _files_of(g)} + dag = PlanDAG(kept) + dag.validate_acyclic() + # The test goes back to the module it tests, which builds on alpha. + assert owner["tests/test_beta.py"] == owner["pkg/beta.py"] + assert owner["pkg/alpha.py"] in dag.ancestors(owner["pkg/beta.py"]) + + def test_an_updated_existing_test_moves_to_the_code_it_follows(self) -> None: + diff = """\ +diff --git a/pkg/engine.py b/pkg/engine.py +--- a/pkg/engine.py ++++ b/pkg/engine.py +@@ -1,2 +1,2 @@ + def compute_score(): +- return 1 ++ return 2 +diff --git a/pkg/other.py b/pkg/other.py +new file mode 100644 +--- /dev/null ++++ b/pkg/other.py +@@ -0,0 +1 @@ ++OTHER_VALUE = 3 +diff --git a/tests/test_engine.py b/tests/test_engine.py +--- a/tests/test_engine.py ++++ b/tests/test_engine.py +@@ -1,2 +1,2 @@ + def test_compute_score(): +- assert compute_score() == 1 ++ assert compute_score() == 2 +""" + head = { + "tests/test_engine.py": ( + "from pkg.engine import compute_score\n" + "from pkg.other import OTHER_VALUE\n" + "def test_compute_score():\n assert compute_score() == 2\n" + ), + "pkg/engine.py": "def compute_score():\n return 2\n", + "pkg/other.py": "OTHER_VALUE = 3\n", + } + groups = [ + _group("pr-1", "pkg/engine.py"), + _group("pr-2", "pkg/other.py"), + _group("pr-3", "tests/test_engine.py"), + ] + kept = make_groups_standalone( + groups, parse_diff(diff), _reader(head), max_loc=3, keep_declared_deps=False + ) + owner = {path: g.id for g in kept for path in _files_of(g)} + # Without it, pr-1's own run of the old test would fail. + assert owner["tests/test_engine.py"] == owner["pkg/engine.py"] + + def test_combined_groups_keep_the_larger_ones_id_and_both_descriptions(self) -> None: + diff = _new_file("pkg/a.py", ["def alpha_one():", " return beta_one()"]) + _new_file( + "pkg/b.py", ["def beta_one():", " return alpha_one()", "", ""] + ) + groups = [_group("pr-1", "pkg/a.py"), _group("pr-2", "pkg/b.py")] + kept = make_groups_standalone(groups, parse_diff(diff)) + assert [g.id for g in kept] == ["pr-2"] + assert kept[0].description == "pr-2\n\npr-1" + assert kept[0].depends_on == [] + + def test_a_planner_s_own_dependencies_are_kept(self) -> None: + diff = _new_file("pkg/a.py", ["A = 1"]) + _new_file("pkg/b.py", ["B = 2"]) + groups = [_group("pr-1", "pkg/a.py"), _group("pr-2", "pkg/b.py", deps=["pr-1"])] + kept = make_groups_standalone(groups, parse_diff(diff)) + assert [g.depends_on for g in kept] == [[], ["pr-1"]] + + def test_a_single_group_is_returned_as_is(self) -> None: + diff = _new_file("pkg/a.py", ["A = 1"]) + groups = [_group("pr-1", "pkg/a.py")] + assert make_groups_standalone(groups, parse_diff(diff)) is groups + + +def test_plan_split_names_a_combined_group_after_what_it_holds() -> None: + from pr_split.config import Settings + from pr_split.constants import PartitionStrategy + from pr_split.planner.client import plan_split + + diff = _new_file("uv.lock", [f"line {i}" for i in range(6)]) + _new_file( + "pyproject.toml", ["[project]", 'name = "x"'] + ) + # A limit of 5 puts the two files in separate groups; they ship together. + settings = Settings(partition_strategy=PartitionStrategy.GRAPH, max_loc=5, min_loc=1) + + groups = plan_split(parse_diff(diff), settings) + + assert len(groups) == 1 + assert groups[0].title == "Add pyproject.toml and 1 more file" + assert groups[0].description == "graph partition over 2 file(s): pyproject.toml, uv.lock" diff --git a/tests/test_partitioning_extensive.py b/tests/test_partitioning_extensive.py index 9731e5f..129bf60 100644 --- a/tests/test_partitioning_extensive.py +++ b/tests/test_partitioning_extensive.py @@ -11,8 +11,8 @@ from pr_split.exceptions import PRSplitError from pr_split.graph import PlanDAG from pr_split.planner.partitioning import ( - _derive_merge_order_dependencies, _test_pair_bonus, + derive_file_order_dependencies, partition_diff, ) from pr_split.planner.scoring import score_plan @@ -379,7 +379,7 @@ def test_merge_order_dependencies_ignore_group_creation_order(self) -> None: ), ] - _derive_merge_order_dependencies(groups) + derive_file_order_dependencies(groups) assert groups[0].depends_on == ["pr-1"] assert groups[1].depends_on == [] diff --git a/tests/test_symbols.py b/tests/test_symbols.py index ab4bd1c..e974acf 100644 --- a/tests/test_symbols.py +++ b/tests/test_symbols.py @@ -1,11 +1,9 @@ from __future__ import annotations -from unittest.mock import patch - from pr_split.constants import AssignmentType from pr_split.diff_ops.parser import parse_diff from pr_split.graph import PlanDAG -from pr_split.planner.partitioning import _add_symbol_dependencies +from pr_split.planner.coherence import make_groups_standalone from pr_split.planner.symbols import symbol_dependencies from pr_split.planner.validator import detect_symbol_order_violations from pr_split.schemas import Group, GroupAssignment @@ -62,25 +60,43 @@ def test_graph_edges_make_the_test_build_on_the_module() -> None: parsed = parse_diff(DIFF) groups = [_whole("pr-1", "tests/test_pruning.py"), _whole("pr-2", "pkg/context_pruning.py")] - _add_symbol_dependencies(groups, parsed) + # 10 LOC together; a limit of 8 keeps them apart, the test after the module. + groups = make_groups_standalone(groups, parsed, max_loc=8) - assert groups[0].depends_on == ["pr-2"] - assert groups[1].depends_on == [] + assert [g.depends_on for g in groups] == [["pr-2"], []] -def test_edge_that_would_close_a_cycle_is_skipped() -> None: +def test_a_test_group_rejoins_the_module_it_tests_when_that_fits() -> None: + parsed = parse_diff(DIFF) + groups = [_whole("pr-1", "tests/test_pruning.py"), _whole("pr-2", "pkg/context_pruning.py")] + + groups = make_groups_standalone(groups, parsed, max_loc=400) + + assert [g.id for g in groups] == ["pr-2"] + assert {a.file_path for a in groups[0].assignments} == { + "pkg/context_pruning.py", + "tests/test_pruning.py", + } + + +def test_groups_that_need_each_other_are_combined() -> None: + # The module already builds on the test group, and the test uses the + # module: no order works, so the two become one group. parsed = parse_diff(DIFF) groups = [ _whole("pr-1", "tests/test_pruning.py"), _whole("pr-2", "pkg/context_pruning.py", depends_on=["pr-1"]), ] - with patch("pr_split.planner.partitioning.logger") as mock_logger: - _add_symbol_dependencies(groups, parsed) + groups = make_groups_standalone(groups, parsed) + assert len(groups) == 1 + assert {a.file_path for a in groups[0].assignments} == { + "tests/test_pruning.py", + "pkg/context_pruning.py", + } assert groups[0].depends_on == [] PlanDAG(groups).validate_acyclic() - assert "would create a cycle" in mock_logger.warning.call_args.args[0] def test_definition_in_a_child_of_its_user_is_reported() -> None: @@ -131,13 +147,13 @@ def test_name_defined_by_two_groups_is_ignored() -> None: def test_graph_partition_links_the_test_group_to_the_module_group() -> None: from pr_split.config import Settings from pr_split.constants import PartitionStrategy - from pr_split.planner.partitioning import partition_diff + from pr_split.planner.client import plan_split parsed = parse_diff(DIFF) # 10 LOC in total; a limit of 8 forces the two files into separate groups. settings = Settings(partition_strategy=PartitionStrategy.GRAPH, max_loc=8, min_loc=1) - groups = partition_diff(parsed, settings) + groups = plan_split(parsed, settings) owner = {a.file_path: g for g in groups for a in g.assignments} test_group = owner["tests/test_pruning.py"] @@ -164,26 +180,75 @@ def test_graph_groups_a_test_file_with_the_module_it_imports() -> None: assert "review-slice" not in groups[0].title -def test_multi_file_group_is_named_after_its_shared_directory() -> None: - from pr_split.planner.partitioning import PartitionUnit, _build_group_title +def _title_of(diff: str, files: list[list[str]]) -> list[str]: + from pr_split.planner.partitioning import retitle_groups - units = [ - PartitionUnit( - id="a", file_path="pkg/edit/rename.py", hunk_indices=(0,), loc=5, position=0 - ), - PartitionUnit(id="b", file_path="pkg/edit/move.py", hunk_indices=(0,), loc=9, position=1), + parsed = parse_diff(diff) + counts = {pf.path: len(pf) for pf in parsed.patch_set} + groups = [ + Group( + id=f"pr-{i}", + title="", + description="", + assignments=[ + GroupAssignment( + file_path=path, + assignment_type=AssignmentType.WHOLE_FILE, + hunk_indices=list(range(counts[path])), + ) + for path in paths + ], + ) + for i, paths in enumerate(files, start=1) ] - assert _build_group_title(3, units) == "chore(split): review edit-3" + retitle_groups(groups, parsed) + return [g.title for g in groups] + + +def test_title_names_the_main_file_and_counts_the_rest() -> None: + titles = _title_of(DIFF, [["pkg/context_pruning.py", "tests/test_pruning.py"]]) + assert titles == ["Add pkg/context_pruning.py and 1 test file"] -def test_multi_file_group_without_a_shared_directory_names_its_largest_file() -> None: - from pr_split.planner.partitioning import PartitionUnit, _build_group_title +def test_title_of_a_single_file_group_has_no_group_number() -> None: + titles = _title_of(DIFF, [["tests/test_pruning.py"], ["pkg/context_pruning.py"]]) + assert titles == ["Add tests/test_pruning.py", "Add pkg/context_pruning.py"] - units = [ - PartitionUnit(id="a", file_path="README.md", hunk_indices=(0,), loc=2, position=0), - PartitionUnit( - id="b", file_path="src/parser_core.py", hunk_indices=(0,), loc=40, position=1 - ), - PartitionUnit(id="c", file_path="setup.cfg", hunk_indices=(0,), loc=1, position=2), + +def test_title_says_which_part_of_a_shared_file_a_group_holds() -> None: + diff = """\ +diff --git a/pkg/core.py b/pkg/core.py +--- a/pkg/core.py ++++ b/pkg/core.py +@@ -1,2 +1,3 @@ + a = 1 ++b = 2 + c = 3 +@@ -20,2 +21,3 @@ + x = 1 ++y = 2 + z = 3 +""" + from pr_split.planner.partitioning import retitle_groups + + parsed = parse_diff(diff) + groups = [ + Group( + id=gid, + title="", + description="", + assignments=[ + GroupAssignment( + file_path="pkg/core.py", + assignment_type=AssignmentType.PARTIAL_HUNKS, + hunk_indices=[idx], + ) + ], + ) + for gid, idx in (("pr-1", 1), ("pr-2", 0)) + ] + retitle_groups(groups, parsed) + assert [g.title for g in groups] == [ + "Update pkg/core.py (part 2 of 2)", + "Update pkg/core.py (part 1 of 2)", ] - assert _build_group_title(2, units) == "chore(split): review parser-core and 2 more-2" From b25cd96604e6d63a13958a1cd569f673496bb319 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 27 Sep 2026 23:23:51 +0000 Subject: [PATCH 2/3] fix: plan wording for one oversized group, and help in the plan editor "pr-1 hold a single hunk each" now reads "pr-1 holds a single hunk" for one group ("pr-1, pr-2 each hold a single hunk" for several). The editor accepts help (or ?) to list its commands again, and an unknown command points to it. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015rnEEjXWDBdePRhKMmZr1w --- README.md | 1 + pr_split/cli.py | 50 +++++++++++++++++++++++--------------- tests/test_cli_coverage.py | 11 +++++++++ tests/test_cli_helpers.py | 21 +++++++++++++--- 4 files changed, 61 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index 912a7e8..fd7eeba 100644 --- a/README.md +++ b/README.md @@ -194,6 +194,7 @@ edit> desc pr-3 Adds the parser and its tests. # set the PR body text edit> new pr-9 # create an empty group to move hunks into edit> merge pr-1 pr-2 # fold pr-2 (hunks, parents, dependants) into pr-1 edit> plan # redisplay the plan table +edit> help # list the commands again edit> done # proceed (default — just press Enter) edit> abort # cancel ``` diff --git a/pr_split/cli.py b/pr_split/cli.py index 89d4cb1..cbed5ca 100644 --- a/pr_split/cli.py +++ b/pr_split/cli.py @@ -371,12 +371,17 @@ def _report_oversized_groups( for g in oversized if sum(len(a.covered_indices(hunk_counts.get(a.file_path, 0))) for a in g.assignments) == 1 ] - irreducible = ( - f" {', '.join(escape(gid) for gid in single_hunk)} hold a single hunk each and" - " cannot be split below the limit." - if single_hunk - else "" - ) + if len(single_hunk) == 1: + irreducible = ( + f" {escape(single_hunk[0])} holds a single hunk and cannot be split below the limit." + ) + elif single_hunk: + irreducible = ( + f" {', '.join(escape(gid) for gid in single_hunk)} each hold a single hunk and" + " cannot be split below the limit." + ) + else: + irreducible = "" console.print( f"[yellow]{len(oversized)} of {len(groups)} groups exceed --max-loc {max_loc}" f" (largest: {escape(largest.id)} at {largest.estimated_loc} LOC).{irreducible}" @@ -1274,6 +1279,21 @@ def _stdin_is_interactive() -> bool: return sys.stdin.isatty() +_EDITOR_HELP = ( + "\n[cyan]Interactive editor. Commands:[/cyan]\n" + " [bold]move[/bold] : \n" + " [bold]movefile[/bold] \n" + " [bold]dep[/bold] / [bold]undep[/bold] \n" + " [bold]title[/bold] / [bold]desc[/bold] \n" + " [bold]new[/bold] / [bold]merge[/bold] \n" + " [bold]show[/bold] \n" + " [bold]plan[/bold] — redisplay the plan table\n" + " [bold]help[/bold] — show these commands again\n" + " [bold]done[/bold] — proceed\n" + " [bold]abort[/bold] — cancel\n" +) + + def _interactive_edit(groups: list[Group], parsed_diff: ParsedDiff) -> list[Group]: if not _stdin_is_interactive(): # Scripts and CI (`split --dry-run < /dev/null`) have no one to answer @@ -1281,18 +1301,7 @@ def _interactive_edit(groups: list[Group], parsed_diff: ParsedDiff) -> list[Grou # before the plan is ever saved. Accept the plan as-is instead. console.print("[yellow]Non-interactive stdin; accepting the plan as-is.[/yellow]") return groups - console.print( - "\n[cyan]Interactive editor. Commands:[/cyan]\n" - " [bold]move[/bold] : \n" - " [bold]movefile[/bold] \n" - " [bold]dep[/bold] / [bold]undep[/bold] \n" - " [bold]title[/bold] / [bold]desc[/bold] \n" - " [bold]new[/bold] / [bold]merge[/bold] \n" - " [bold]show[/bold] \n" - " [bold]plan[/bold] — redisplay the plan table\n" - " [bold]done[/bold] — proceed\n" - " [bold]abort[/bold] — cancel\n" - ) + console.print(_EDITOR_HELP) while True: try: cmd = typer.prompt("edit", default="done") @@ -1375,9 +1384,12 @@ def _interactive_edit(groups: list[Group], parsed_diff: ParsedDiff) -> list[Grou continue if _merge_groups(groups, parsed_diff, parts[1], parts[2]): recompute_estimated_loc(groups, parsed_diff) + elif action in ("help", "?"): + console.print(_EDITOR_HELP) else: console.print( - "[yellow]Unknown command. Type 'done' to proceed or 'abort' to cancel.[/yellow]" + "[yellow]Unknown command. Type 'help' for the commands, 'done' to proceed" + " or 'abort' to cancel.[/yellow]" ) diff --git a/tests/test_cli_coverage.py b/tests/test_cli_coverage.py index 2805e6a..caa1b25 100644 --- a/tests/test_cli_coverage.py +++ b/tests/test_cli_coverage.py @@ -2496,6 +2496,17 @@ def test_dep_title_desc_and_undep(self, mock_prompt: MagicMock) -> None: groups = _interactive_edit(groups, parsed) assert groups[1].depends_on == [] + @patch("pr_split.cli.typer.prompt") + def test_help_lists_the_commands_again(self, mock_prompt: MagicMock) -> None: + from pr_split.cli import console + + mock_prompt.side_effect = ["help", "done"] + with console.capture() as capture: + _interactive_edit(self._two_groups(), parse_diff(TWO_HUNK_DIFF)) + out = capture.get() + assert out.count("Interactive editor. Commands:") == 2 + assert "Unknown command" not in out + @patch("pr_split.cli.typer.prompt") def test_dep_that_would_cycle_is_refused(self, mock_prompt: MagicMock) -> None: groups = self._two_groups() diff --git a/tests/test_cli_helpers.py b/tests/test_cli_helpers.py index 3e12c05..45f24b3 100644 --- a/tests/test_cli_helpers.py +++ b/tests/test_cli_helpers.py @@ -999,7 +999,22 @@ def test_single_hunk_group_is_named_as_irreducible(self) -> None: with console.capture() as capture: _report_oversized_groups([big_file], 400, {"tests/test_big.py": 1}) out = " ".join(capture.get().split()) - assert "pr-1 hold a single hunk each and cannot be split below the limit" in out + assert "pr-1 holds a single hunk and cannot be split below the limit" in out + + def test_several_single_hunk_groups_each_hold_one(self) -> None: + from pr_split.cli import _report_oversized_groups, console + + groups = [] + for gid, path in (("pr-1", "a.py"), ("pr-2", "b.py")): + group = self._sized(gid, 500) + group.assignments = [ + GroupAssignment(file_path=path, assignment_type=AssignmentType.WHOLE_FILE) + ] + groups.append(group) + with console.capture() as capture: + _report_oversized_groups(groups, 400, {"a.py": 1, "b.py": 1}) + out = " ".join(capture.get().split()) + assert "pr-1, pr-2 each hold a single hunk and cannot be split" in out def test_whole_file_assignments_count_the_hunks_they_cover(self) -> None: from pr_split.cli import _report_oversized_groups, console @@ -1018,8 +1033,8 @@ def test_whole_file_assignments_count_the_hunks_they_cover(self) -> None: with console.capture() as capture: _report_oversized_groups([new_file, edited], 400, {"big.py": 1, "multi.py": 3}) out = " ".join(capture.get().split()) - assert "pr-1 hold a single hunk each" in out - assert "pr-2 hold" not in out and "pr-1, pr-2" not in out + assert "pr-1 holds a single hunk" in out + assert "pr-2 holds" not in out and "pr-1, pr-2" not in out def test_nothing_printed_within_the_limit(self) -> None: from pr_split.cli import _report_oversized_groups, console From 984b68c4ce4453e86136f82f77142c450cd96bc6 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 27 Sep 2026 23:31:03 +0000 Subject: [PATCH 3/3] fix: resolve src/-layout imports and keep peeled test-group ids unique Absolute imports now also resolve under src/, so a test of src/pkg/mod.py still builds on the group changing it. A test group peeled off an LLM group gets a suffixed id when -tests is already taken, instead of a duplicate id that fails plan validation. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015rnEEjXWDBdePRhKMmZr1w --- pr_split/planner/coherence.py | 16 ++++++++++++++-- tests/test_coherence.py | 24 ++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/pr_split/planner/coherence.py b/pr_split/planner/coherence.py index 8875176..e2314b4 100644 --- a/pr_split/planner/coherence.py +++ b/pr_split/planner/coherence.py @@ -123,9 +123,15 @@ def _hunk_lines(parsed_diff: ParsedDiff, path: str) -> list[str]: return [] -def _module_files(module: str) -> tuple[str, str]: +# Where absolute imports resolve from: the repository root, or a src/ layout. +_SOURCE_ROOTS = ("", "src/") + + +def _module_files(module: str) -> tuple[str, ...]: base = module.replace(".", "/") - return f"{base}.py", f"{base}/__init__.py" + return tuple( + f"{root}{base}{suffix}" for root in _SOURCE_ROOTS for suffix in (".py", "/__init__.py") + ) def _resolve_relative(path: str, module: str) -> str: @@ -829,6 +835,7 @@ def _peel_tests(groups: list[Group], keep_declared_deps: bool) -> list[Group]: """ numbered = all(re.fullmatch(r"pr-\d+", g.id) for g in groups) next_number = max((int(g.id[3:]) for g in groups), default=0) + 1 if numbered else 0 + taken = {g.id for g in groups} peeled: list[Group] = [] for group in groups: tests = [a for a in group.assignments if is_test_path(a.file_path)] @@ -841,6 +848,11 @@ def _peel_tests(groups: list[Group], keep_declared_deps: bool) -> list[Group]: next_number += 1 else: test_id = f"{group.id}-tests" + suffix = 2 + while test_id in taken: # a planner may already use that id + test_id = f"{group.id}-tests-{suffix}" + suffix += 1 + taken.add(test_id) test_group = group.model_copy( update={ "id": test_id, diff --git a/tests/test_coherence.py b/tests/test_coherence.py index 7ac3bb9..443e4e0 100644 --- a/tests/test_coherence.py +++ b/tests/test_coherence.py @@ -86,6 +86,15 @@ def test_imports_are_followed_through_unchanged_modules(self) -> None: needs = find_needs(groups, parse_diff(diff), _reader(head)) assert [(n.user, n.provider) for n in needs] == [("pr-2", "pr-1")] + def test_imports_resolve_in_a_src_layout(self) -> None: + # No shared name: only resolving the import to src/ links the two. + diff = _new_file("src/pkg/engine.py", ["x = 1"]) + _new_file( + "tests/test_widgets.py", ["import pkg.engine"] + ) + groups = [_group("pr-1", "src/pkg/engine.py"), _group("pr-2", "tests/test_widgets.py")] + needs = find_needs(groups, parse_diff(diff)) + assert [(n.user, n.provider) for n in needs] == [("pr-2", "pr-1")] + def test_tests_need_the_conftest_above_them(self) -> None: diff = _new_file("tests/conftest.py", ["import pytest"]) + _new_file( "tests/unit/test_a.py", ["def test_a():", " pass"] @@ -333,6 +342,21 @@ def test_a_planner_s_own_dependencies_are_kept(self) -> None: kept = make_groups_standalone(groups, parse_diff(diff)) assert [g.depends_on for g in kept] == [[], ["pr-1"]] + def test_a_peeled_test_group_never_reuses_an_existing_id(self) -> None: + diff = ( + _new_file("pkg/core.py", ["CORE_VALUE = 1"]) + + _new_file("tests/test_core.py", ["from pkg.core import CORE_VALUE"]) + + _new_file("docs/core.md", ["# Core"]) + ) + groups = [ + _group("core", "pkg/core.py", "tests/test_core.py"), + _group("core-tests", "docs/core.md"), + ] + kept = make_groups_standalone(groups, parse_diff(diff), max_loc=1) + ids = [g.id for g in kept] + assert len(ids) == len(set(ids)) + assert "core-tests-2" in ids + def test_a_single_group_is_returned_as_is(self) -> None: diff = _new_file("pkg/a.py", ["A = 1"]) groups = [_group("pr-1", "pkg/a.py")]