refactor(add): replace #N PR syntax with --pr flag#9
Conversation
The #N syntax was problematic because shells interpret # as a comment.
Users had to quote it ('grove add "#123"') which was unintuitive.
The --pr flag is explicit and doesn't require shell escaping.
There was a problem hiding this comment.
Pull request overview
This PR refactors the grove add command to replace the problematic #N syntax with an explicit --pr flag, eliminating the need for shell escaping when referencing pull requests by number.
Key Changes:
- Replaced
grove add #123withgrove add --pr 123syntax - Added validation and helpful error messages for the old syntax and edge cases
- Updated all tests and documentation to reflect the new flag-based approach
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
cmd/grove/commands/add.go |
Implements the new --pr flag, adds validation logic for PR numbers, detects and provides migration guidance for old #N syntax |
cmd/grove/commands/add_test.go |
Updates all unit tests to use the new --pr flag syntax and adds tests for new validation cases |
cmd/grove/testdata/script/add_validation.txt |
Updates validation tests to verify the new --pr flag behavior and error messages |
cmd/grove/testdata/script/add_pr_integration.txt |
Updates integration tests to use --pr flag instead of #N syntax |
README.md |
Updates documentation and examples to show the new --pr flag usage |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if prNumber < 0 { | ||
| return fmt.Errorf("--pr must be a positive number") | ||
| } |
There was a problem hiding this comment.
The validation for zero PR numbers produces a confusing error message. When a user runs grove add --pr 0, the check at line 70 passes (since 0 is not less than 0), but line 75 treats it as if the flag wasn't provided (prFlag = false). This causes line 85 to return "requires branch, PR URL, or --pr flag" even though the user did provide --pr. The validation at line 70 should check prNumber <= 0 to catch this case explicitly and provide a clear error message.
| return fmt.Errorf("'%s' syntax no longer supported, use: grove add --pr %s", | ||
| branchOrPR, strings.TrimPrefix(branchOrPR, "#")) |
There was a problem hiding this comment.
The error message for the old #N syntax is incomplete when handling edge cases. If a user enters just # without a number, the error message will be malformed: '#' syntax no longer supported, use: grove add --pr (with nothing after --pr). The code should validate that there's actually a number after the # before suggesting the migration, or check against the old regex pattern that required digits.
| return fmt.Errorf("'%s' syntax no longer supported, use: grove add --pr %s", | |
| branchOrPR, strings.TrimPrefix(branchOrPR, "#")) | |
| prStr := strings.TrimPrefix(branchOrPR, "#") | |
| if prStr != "" { | |
| allDigits := true | |
| for _, ch := range prStr { | |
| if ch < '0' || ch > '9' { | |
| allDigits = false | |
| break | |
| } | |
| } | |
| if allDigits { | |
| return fmt.Errorf("'%s' syntax no longer supported, use: grove add --pr %s", | |
| branchOrPR, prStr) | |
| } | |
| } |
Replace
grove add #123syntax withgrove add --pr 123flag.The
#Nsyntax was problematic because shells interpret#as a comment character, requiring users to quote it (grove add '#123'). The--prflag is explicit and doesn't require shell escaping.Changes
grove add #123withgrove add --pr 123flag#Nsyntax--pr -5now errors clearly)Test plan
grove add --pr 7creates worktree for PR test: integration test PR (DO NOT MERGE) #7grove add '#123'shows:'#123' syntax no longer supported, use: grove add --pr 123grove add --pr -5shows:--pr must be a positive numbergrove add --pr 123 featureshows:--pr flag cannot be combined with positional argument