Make but review publish take a CliId instead of requiring -b branch
#11040
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
@copilot continue |
|
@copilot continue |
The implementation is complete and ready for review. The PR description has been updated to remove the WIP tag. Summary of changes:
The implementation is ready for testing and merging. |
but review publish take a CliId instead of requiring -b branch
Co-authored-by: Byron <[email protected]>
Byron
force-pushed
the
copilot/fix-broken-link-in-documentation
branch
from
98a7836 to
2335287
Compare
Byron
left a comment
Byron
left a comment
There was a problem hiding this comment.
I think this implementation will do the trick, but this command has no tests.
So I leave it to @schacon and @krlvi to give it an in-person shot.
If we think this is good, I think we should have a lot of tests for CliIds, and make it (more) independent of the Context. It shouldn't need more than a gix::Repository for most operations (which would make it very testable).
Thanks
| if cli_ids.is_empty() { | ||
| anyhow::bail!( | ||
| "Branch '{}' not found. If you just performed a Git operation (squash, rebase, etc.), try running 'but status' to refresh the current state.", | ||
| branch_input | ||
| ); | ||
| } | ||
|
|
||
| if cli_ids.len() > 1 { | ||
| let matches: Vec<String> = cli_ids | ||
| .iter() | ||
| .map(|id| match id { | ||
| crate::id::CliId::Branch { name } => format!("{id} (branch '{name}')"), | ||
| _ => format!("{} ({})", id, id.kind()), | ||
| }) | ||
| .collect(); | ||
| anyhow::bail!( | ||
| "Branch identifier '{}' is ambiguous. Matches: {}. Try using more characters, a longer identifier, or the full branch name to disambiguate.", | ||
| branch_input, | ||
| matches.join(", ") | ||
| ); | ||
| } | ||
|
|
||
| // Extract branch name from the resolved CliId | ||
| match cli_ids.pop().unwrap() { |
There was a problem hiding this comment.
@krlvi This seems like a pretty general implementation for "we want a branch by CliId" - should we have a reusable function for this?
Make
but review publishtake a CliId instead of requiring-b branchImplementation Summary:
This PR implements the requested feature from issue #11027 to make
but review publishaccept a CliId as a positional argument, consistent with how therubcommand works.Changes Made:
Modified command structure (
forge/review.rs):branchfield from a flag (-b/--branch) to a positional optional argumentAdded CliId resolution (
forge/review.rs):resolve_branch_name()helper function with comprehensive documentation that:CliId::from_str()to resolve branch input (handles both exact names and partial matches)rubcommandUpdated publish logic (
forge/review.rs):publish_reviews()to callresolve_branch_name()when a branch is specifiedCode Review Feedback Addressed:
New Behavior:
but review publish- publishes reviews for all active branches (unchanged)but review publish branch-name- publishes review for the specified branch (new positional argument)but review publish <CliId>- publishes review for the branch matching the CliId (new feature)rubcommand)Error Handling:
The implementation includes comprehensive error handling:
All error messages are consistent with the existing
rubcommand patterns.Testing:
cargo fmtOriginal prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.