Refactor error handling in worktree deletion process#12
Conversation
- Introduced a constant `EXIT_ERROR` for consistent error exit codes. - Updated the confirmation prompt for deleting worktrees with uncommitted changes to use an error message instead of a confirmation dialog. - Clarified comments regarding base branch determination logic. These changes improve the robustness and clarity of the worktree management functionality.
There was a problem hiding this comment.
Pull request overview
This PR aims to refactor error handling in the worktree deletion process by introducing a constant for exit codes, changing the confirmation flow for uncommitted changes, and clarifying comments about base branch logic.
Changes:
- Introduced
EXIT_ERRORconstant for consistent error exit codes - Modified uncommitted changes handling to use
error()instead ofconfirm()dialog - Updated comment for base branch determination logic to clarify when it's used
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tools/wt-worktree/wt/worktree.py | Adds EXIT_ERROR constant, refactors uncommitted changes check in delete_worktree to use error() function, and clarifies base branch comment in create_worktree |
| tools/wt-worktree/uv.lock | Newly added dependency lock file with package specifications and checksums |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "use --force to delete anyway", | ||
| EXIT_ERROR) | ||
| return |
There was a problem hiding this comment.
The error() function is being called with EXIT_ERROR which will call sys.exit(1), causing the program to terminate immediately. This means the return statement on line 225 will never be executed. Additionally, this creates inconsistent behavior compared to the unpushed commits check below (lines 241-246), which properly returns False when the user doesn't confirm deletion. The error handling should be refactored to either return False (allowing the caller to handle the response) or consistently use sys.exit for all validation failures.
| "use --force to delete anyway", | |
| EXIT_ERROR) | |
| return | |
| "use --force to delete anyway" | |
| ) | |
| return False |
| # Determine base branch | ||
| if base is None: | ||
| # Determine base branch (only used when creating a new branch) | ||
| if base is None and not use_existing_branch: |
There was a problem hiding this comment.
The variable use_existing_branch is referenced but is not defined in the method signature or anywhere in the function. This will cause a NameError at runtime. The function signature shows create_worktree(self, name: str, base: Optional[str] = None, detached: bool = False), so this variable should either be added as a parameter or replaced with the correct logic (possibly checking if create_branch is False).
| if base is None and not use_existing_branch: | |
| if base is None and create_branch: |
| return False | ||
| "use --force to delete anyway", | ||
| EXIT_ERROR) | ||
| return |
There was a problem hiding this comment.
The function signature indicates a return type of bool (line 187), but when uncommitted changes are detected without force, the function now returns None (line 225) instead of False. This creates an inconsistency with the documented return type and the calling code expectations. The return statement should be return False to match the type hint and maintain consistency with the unpushed commits check on line 246.
| return | |
| return False |
EXIT_ERRORfor consistent error exit codes.These changes improve the robustness and clarity of the worktree management functionality.