feat: support custom PR body template via .pr-split/template.md#23
feat: support custom PR body template via .pr-split/template.md#23
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the flexibility of pull request generation by enabling users to define their own custom templates for PR bodies. This allows for greater control over the presentation of PR information, promoting consistency and tailored communication without altering the core logic. The change introduces a new configuration point and a robust templating engine that integrates seamlessly with existing PR data. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature for customizing PR body content using a template file. The implementation is clean and correctly falls back to the default format if no template is provided. My main suggestion is to improve the robustness of the template processing by adding error handling for invalid placeholders. This will prevent crashes and provide better feedback to users when they make a mistake in their template file.
pr_split/cli.py
Outdated
| if _PR_TEMPLATE_PATH.exists(): | ||
| template = _PR_TEMPLATE_PATH.read_text(encoding="utf-8") | ||
| return template.format(**template_vars) |
There was a problem hiding this comment.
Using template.format(**template_vars) can lead to a KeyError if the user's template file contains an invalid or misspelled placeholder. This would crash the program with an unfriendly traceback. It's better to handle this potential error and provide a helpful message to the user, guiding them on how to fix their template.
I suggest wrapping the format call in a try...except block to catch KeyError and raise a PRSplitError with a clear error message listing the available placeholders.
if _PR_TEMPLATE_PATH.exists():
template = _PR_TEMPLATE_PATH.read_text(encoding="utf-8")
try:
return template.format(**template_vars)
except KeyError as e:
available_keys = ", ".join(f"{{{k}}}" for k in sorted(template_vars.keys()))
msg = (
f"Error in PR template '{_PR_TEMPLATE_PATH}': invalid placeholder {e}.\n"
f"Available placeholders are: {available_keys}"
)
raise PRSplitError(msg) from e
Greptile SummaryThis PR adds support for a user-defined PR body template at
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["_build_pr_body(group, all_groups)"] --> B{{"_PR_TEMPLATE_PATH.exists()?"}}
B -- Yes --> C["Build template_vars dict\n(description, files, added, removed,\nloc, dependencies, dag, id, title)"]
C --> D["_PR_TEMPLATE_PATH.read_text()"]
D -- OSError --> E["raise PRSplitError\n'Could not read PR template'"]
D -- success --> F["template.format(**template_vars)"]
F -- KeyError / ValueError\n/ IndexError --> G["raise PRSplitError\n'Invalid PR template'\n+ list available placeholders"]
F -- success --> H["return formatted body"]
B -- No --> I["Build default sections list\n(description, files, diff stats,\ndependencies, DAG)"]
I --> J["return '\\n\\n'.join(sections)"]
|
2a7bc8a to
333a711
Compare
|
@greptile |
|
@greptile |
1 similar comment
|
@greptile |
|
@greptile |
|
@greptile |
|
@greptileai review |
| plan_path = Path(PLAN_FILE) | ||
| if plan_path.exists(): | ||
| plan_path.unlink() |
There was a problem hiding this 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:
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.
Summary
.pr-split/template.mdwith Python format placeholders to customize PR bodyExample template:
{description} ### Changed files {files} **Stats:** +{added}/-{removed} ({loc} lines) {dag}Test plan
.pr-split/template.md, run split, verify custom PR body