Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 38 additions & 11 deletions pr_split/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -260,28 +260,55 @@ def _create_branches_and_commits(
_gh_semaphore = Semaphore(_GH_API_CONCURRENCY)


_PR_TEMPLATE_PATH = Path(PLAN_DIR) / "template.md"


def _build_pr_body(group: Group, all_groups: list[Group]) -> str:
sections = [group.description]
if _PR_TEMPLATE_PATH.exists():
files = [a.file_path for a in group.assignments]
template_vars = {
"description": group.description,
"files": "\n".join(f"- `{f}`" for f in files),
"added": group.estimated_added,
"removed": group.estimated_removed,
"loc": group.estimated_loc,
"dependencies": ", ".join(f"`{d}`" for d in group.depends_on),
"dag": _render_dag_markdown(all_groups, group.id),
"id": group.id,
"title": group.title,
}
try:
template = _PR_TEMPLATE_PATH.read_text(encoding="utf-8")
return template.format(**template_vars)
except (KeyError, ValueError, IndexError) as exc:
available = ", ".join(
f"{{{k}}}" for k in sorted(template_vars)
)
raise PRSplitError(
f"Invalid PR template at {_PR_TEMPLATE_PATH}: {exc}. "
f"Available placeholders: {available}. "
"Escape literal braces with {{ and }}."
) from exc
except OSError as exc:
raise PRSplitError(
f"Could not read PR template at {_PR_TEMPLATE_PATH}: {exc}"
) from exc

files = [a.file_path for a in group.assignments]
sections = [group.description]
if files:
file_list = "\n".join(f"- `{f}`" for f in files)
sections.append(f"## Files changed\n\n{file_list}")

sections.append(
f"## Diff stats\n\n"
f"**+{group.estimated_added}** additions, "
f"**-{group.estimated_removed}** deletions "
f"({group.estimated_loc} LOC)"
)

deps = group.depends_on
if deps:
dep_list = ", ".join(f"`{d}`" for d in deps)
if group.depends_on:
dep_list = ", ".join(f"`{d}`" for d in group.depends_on)
sections.append(f"## Dependencies\n\nThis PR depends on: {dep_list}")

sections.append(_render_dag_markdown(all_groups, group.id))

return "\n\n".join(sections)


Expand Down Expand Up @@ -541,9 +568,9 @@ def _cleanup_git_state(git_state: GitState) -> tuple[int, int]:
except PRSplitError:
logger.warning(f"Could not delete branch {branch_record.branch_name}")

plan_dir = Path(PLAN_DIR)
if plan_dir.exists():
shutil.rmtree(plan_dir)
plan_path = Path(PLAN_FILE)
if plan_path.exists():
plan_path.unlink()
Comment on lines +571 to +573
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Orphaned .pr-split/ directory after clean

The previous implementation removed the entire .pr-split/ directory with shutil.rmtree. The new code only deletes plan.json, which preserves any user template.md — but when no template exists, an empty .pr-split/ directory is left on disk after clean.

This is a silent behavioural regression for users who don't have a template: they now get a stale empty directory rather than a clean slate. The directory will be silently recreated on the next run (mkdir(exist_ok=True) in save_plan), so it's not harmful, but it's surprising and potentially confusing if users track .pr-split/ in their .gitignore or inspect it manually.

Consider removing the directory if it would be left empty after the plan file is deleted:

plan_path = Path(PLAN_FILE)
if plan_path.exists():
    plan_path.unlink()

plan_dir = Path(PLAN_DIR)
if plan_dir.exists() and not any(plan_dir.iterdir()):
    plan_dir.rmdir()

This preserves a template.md (directory is non-empty → not removed) while still giving a clean state when no template exists.

Prompt To Fix With AI
This is a comment left during a code review.
Path: pr_split/cli.py
Line: 571-573

Comment:
**Orphaned `.pr-split/` directory after `clean`**

The previous implementation removed the entire `.pr-split/` directory with `shutil.rmtree`. The new code only deletes `plan.json`, which preserves any user `template.md` — but when no template exists, an empty `.pr-split/` directory is left on disk after `clean`.

This is a silent behavioural regression for users who don't have a template: they now get a stale empty directory rather than a clean slate. The directory will be silently recreated on the next run (`mkdir(exist_ok=True)` in `save_plan`), so it's not harmful, but it's surprising and potentially confusing if users track `.pr-split/` in their `.gitignore` or inspect it manually.

Consider removing the directory if it would be left empty after the plan file is deleted:

```python
plan_path = Path(PLAN_FILE)
if plan_path.exists():
    plan_path.unlink()

plan_dir = Path(PLAN_DIR)
if plan_dir.exists() and not any(plan_dir.iterdir()):
    plan_dir.rmdir()
```

This preserves a `template.md` (directory is non-empty → not removed) while still giving a clean state when no template exists.

How can I resolve this? If you propose a fix, please make it concise.


return closed_prs, deleted_branches

Expand Down
31 changes: 31 additions & 0 deletions tests/test_cli_new_features.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
from __future__ import annotations

from pathlib import Path
from unittest.mock import MagicMock, patch

import pytest

from pr_split.cli import _build_pr_body, _cleanup_git_state
from pr_split.constants import AssignmentType
from pr_split.exceptions import GitOperationError, PRSplitError
Expand Down Expand Up @@ -84,6 +87,34 @@ def test_no_files_section_when_no_assignments(self) -> None:
body = _build_pr_body(group, [group])
assert "## Files changed" not in body

def test_custom_template(
self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch
) -> None:
template_dir = tmp_path / ".pr-split"
template_dir.mkdir()
template_file = template_dir / "template.md"
template_file.write_text("{description}\n\nFiles: {files}\nLOC: {loc}")
monkeypatch.setattr("pr_split.cli._PR_TEMPLATE_PATH", template_file)

group = _group("pr-1", "feat: auth", files=["auth.py"], added=10, removed=5)
body = _build_pr_body(group, [group])
assert "desc for pr-1" in body
assert "auth.py" in body
assert "15" in body

def test_custom_template_invalid_placeholder(
self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch
) -> None:
template_dir = tmp_path / ".pr-split"
template_dir.mkdir()
template_file = template_dir / "template.md"
template_file.write_text("{nonexistent}")
monkeypatch.setattr("pr_split.cli._PR_TEMPLATE_PATH", template_file)

group = _group("pr-1", "feat: auth", files=["auth.py"])
with pytest.raises(PRSplitError, match="Invalid PR template"):
_build_pr_body(group, [group])


class TestCleanupGitState:
@patch("pr_split.cli.shutil.rmtree")
Expand Down
Loading