feat: add interactive plan editor to move hunks between groups#24
feat: add interactive plan editor to move hunks between groups#24
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 significantly enhances the user experience for managing pull request splits by integrating an interactive command-line editor. This new feature empowers users to review and modify the proposed grouping of code changes (hunks) in real-time, providing granular control over the final split plan. It allows for more precise organization of changes, ensuring that each resulting pull request is logically coherent before creation. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an interactive editor for modifying the split plan, which is a great feature. The implementation is well-structured, but I found a critical issue in the logic for moving hunks between groups. The current implementation doesn't handle all assignment types, which will lead to errors for the user. My review comment provides details on the issue and a path to resolution.
| def _move_assignment( | ||
| groups: list[Group], file_path: str, hunk_index: int, from_id: str, to_id: str | ||
| ) -> bool: | ||
| group_map = {g.id: g for g in groups} | ||
| src = group_map.get(from_id) | ||
| dst = group_map.get(to_id) | ||
| if not src or not dst: | ||
| console.print(f"[red]Group '{from_id}' or '{to_id}' not found.[/red]") | ||
| return False | ||
|
|
||
| found = False | ||
| for assignment in src.assignments: | ||
| if assignment.file_path == file_path and hunk_index in assignment.hunk_indices: | ||
| assignment.hunk_indices.remove(hunk_index) | ||
| if not assignment.hunk_indices: | ||
| src.assignments.remove(assignment) | ||
| found = True | ||
| break | ||
|
|
||
| if not found: | ||
| console.print(f"[red]Hunk {file_path}:{hunk_index} not found in {from_id}.[/red]") | ||
| return False | ||
|
|
||
| dst_assignment = next( | ||
| (a for a in dst.assignments if a.file_path == file_path), None | ||
| ) | ||
| if dst_assignment: | ||
| dst_assignment.hunk_indices.append(hunk_index) | ||
| dst_assignment.hunk_indices.sort() | ||
| else: | ||
| dst.assignments.append( | ||
| GroupAssignment( | ||
| file_path=file_path, | ||
| assignment_type=AssignmentType.PARTIAL_HUNKS, | ||
| hunk_indices=[hunk_index], | ||
| ) | ||
| ) | ||
|
|
||
| console.print(f"[green]Moved {file_path}:{hunk_index} from {from_id} to {to_id}[/green]") | ||
| return True |
There was a problem hiding this comment.
The _move_assignment function has a couple of issues that make it fragile and functionally incomplete.
-
Unsafe list modification: The loop starting at line 350 modifies
src.assignmentswhile iterating over it. While thebreakstatement prevents bugs in this specific case, it's a fragile pattern that should be avoided. It's safer to first find the item to modify/remove, and then perform the operation. -
WHOLE_FILEassignments not handled: The logic only checks forhunk_index in assignment.hunk_indices. This will fail for assignments of typeAssignmentType.WHOLE_FILE, wherehunk_indicesis not populated. A user trying to move a hunk from a file that was wholly assigned to a group will find that the command fails with a "hunk not found" error. This is a significant functional gap for the interactive editor.
To fix this, _move_assignment needs to be aware of the total number of hunks in a file. This information is in ParsedDiff. This requires a few changes:
- The
splitfunction should passparsed_diffto_interactive_editat line 546:groups = _interactive_edit(groups, parsed_diff). - The
_interactive_editfunction signature must be updated to acceptparsed_diffand pass it to_move_assignment. - This function's (
_move_assignment) signature must be updated to acceptparsed_diff.
With parsed_diff available, you can refactor this function to correctly handle all cases. A robust implementation would:
- Find the source assignment for the given
file_path. - Get the
patch_filefromparsed_diffto know thetotal_hunks. - Check if the
hunk_indexis valid and present in the source assignment, considering bothWHOLE_FILEandPARTIAL_HUNKStypes. - If it's a
WHOLE_FILEassignment, convert it toPARTIAL_HUNKSwith all hunks except the one being moved. - Remove the hunk from the source assignment's
hunk_indices. - If the source assignment becomes empty, remove it from the group.
- Add the hunk to the destination group's assignment, creating one if necessary.
Greptile SummaryThis PR introduces an interactive plan editor that lets users inspect and reorganise hunk assignments before committing to a split. After the initial plan is displayed, Several rounds of fix-up commits have addressed the majority of previously identified issues: the self-move guard, Remaining concerns (already flagged in earlier review threads, not yet resolved):
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[split command] --> B[plan_split → groups]
B --> C[validate_plan pre-edit]
C --> D[_present_plan]
D --> E[_interactive_edit loop]
E --> F{Read command}
F -- done / Enter --> G[return groups]
F -- abort / Ctrl-C --> H[typer.Abort]
F -- plan --> I[_present_plan groups]
I --> F
F -- show group_id --> J[_show_group_detail]
J --> F
F -- move file:n from to --> K{Validate args}
K -- invalid --> L[print usage error]
L --> F
K -- valid --> M[_move_assignment]
M --> N{from_id == to_id?}
N -- yes --> O[warn, return False]
O --> F
N -- no --> P{src/dst groups found?}
P -- no --> Q[error, return False]
Q --> F
P -- yes --> R{src assignment WHOLE_FILE?}
R -- yes --> S[expand hunk_indices, downgrade to PARTIAL_HUNKS]
S --> T{hunk in indices?}
R -- no --> T
T -- no --> U[Hunk not found error]
U --> F
T -- yes --> V[remove hunk from src]
V --> W{src assignment empty?}
W -- yes --> X[remove assignment from src]
W -- no --> Y[dst: find/create assignment]
X --> Y
Y --> Z[add hunk_index to dst, sort]
Z --> AA[print success]
AA --> F
G --> AB{empty groups?}
AB -- yes --> AC[Exit 1]
AB -- no --> AD[validate_plan post-edit]
AD --> AE{PRSplitError?}
AE -- yes --> AF[print error, Exit 1]
AE -- no --> AG[continue → confirm → create PRs]
Reviews (6): Last reviewed commit: "fix: reject negative hunk indices, preve..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
vitali87
force-pushed
the
feat/interactive-plan-editor
branch
from
abf9c6f to
256396b
Compare
|
@greptile |
…duplicate hunk indices
|
@greptile |
|
@greptile |
| except (KeyboardInterrupt, EOFError) as exc: | ||
| raise typer.Abort() from exc | ||
|
|
||
| parts = cmd.strip().split() |
There was a problem hiding this comment.
move command
cmd.strip().split() splits on all whitespace without respecting quoting. A file path like src/my components/foo.py would produce 5 tokens instead of 4, triggering the "wrong argument count" error rather than a helpful "file paths with spaces must be quoted" message. Consider using shlex.split to support standard shell quoting conventions:
import shlex
...
try:
parts = shlex.split(cmd.strip())
except ValueError:
parts = cmd.strip().split()This allows users to write move "src/my components/foo.py":0 g1 g2 and have it parsed correctly.
Prompt To Fix With AI
This is a comment left during a code review.
Path: pr_split/cli.py
Line: 447
Comment:
**File paths with spaces silently break the `move` command**
`cmd.strip().split()` splits on all whitespace without respecting quoting. A file path like `src/my components/foo.py` would produce 5 tokens instead of 4, triggering the "wrong argument count" error rather than a helpful "file paths with spaces must be quoted" message. Consider using `shlex.split` to support standard shell quoting conventions:
```python
import shlex
...
try:
parts = shlex.split(cmd.strip())
except ValueError:
parts = cmd.strip().split()
```
This allows users to write `move "src/my components/foo.py":0 g1 g2` and have it parsed correctly.
How can I resolve this? If you propose a fix, please make it concise.
|
@greptile |
…alid moves, catch validation errors
|
@greptile |
|
@greptileai review |
1 participant
Summary
move <file>:<hunk_index> <from_group> <to_group>— move a hunk between groupsshow <group_id>— display group details (files, hunks, deps, stats)plan— redisplay the plan tabledone— proceed to confirmationabort— canceldone(just press Enter to skip editing)Test plan
showto inspect groups,movea hunk,planto verify,doneto proceedabortto cancel