-
Notifications
You must be signed in to change notification settings - Fork 751
better base check (json and human) #11604
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR enhances the but base check command by implementing proper JSON output support (previously a no-op), correcting the display to show upstream commits instead of recent commits, and updating the output styling to use simpler ANSI colors instead of emojis.
Key Changes:
- Implemented functional
--jsonflag with structured output including base branch info, upstream commits, and branch statuses - Fixed commit display to show
upstream_commitsrather thanrecent_commits - Modernized human-readable output with ANSI color styling (dimmed, colored status indicators) replacing emoji-heavy format
Byron
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is just one comment from me, and maybe the request for a test as I don't think there is one yet. Pinning the output now to something you like will help to keep it that way later once the command is 'modernised'.
| /// JSON output for `but base check` | ||
| #[derive(Serialize)] | ||
| #[serde(rename_all = "camelCase")] | ||
| struct BaseCheckOutput { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these would be better when placed in a json module (I think it's also done in but status).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Byron split this into base/mod.rs and base/json.rs
6744278 to
4842d64
Compare
4842d64 to
1ab0162
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| writeln!(out, "✅ Everything is up to date")?; | ||
| } | ||
| None | ||
| } | ||
| UpdatesRequired { | ||
| worktree_conflicts, | ||
| statuses, | ||
| } => { | ||
| if !worktree_conflicts.is_empty() { | ||
| if let Some(out) = out.for_human() { | ||
| writeln!( | ||
| out, | ||
| "❗️ There are uncommitted changes in the worktree that may conflict with | ||
| the updates. Please commit or stash them and try again." | ||
| )?; | ||
| } | ||
| None | ||
| } else { | ||
| if let Some(out) = out.for_human() { | ||
| writeln!(out, "🔄 Updating branches...")?; |
Copilot
AI
Dec 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Update subcommand still uses emoji-based output (✅, ❗️, 🔄) while the Check subcommand has been updated to use ANSI colored text. For consistency with the PR's stated goal of preferring "simpler, ansi colored style" over "emoji laden" style, consider updating the Update subcommand's output to match the new Check subcommand style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot open a new pull request to apply changes based on this feedback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
71f1280 to
d150b14
Compare
Provide a machine-readable JSON representation of the `but base check` command so callers can programmatically consume base branch info, upstream commits and branch mergeability status. Introduce serializable structs (BaseCheckOutput, BaseBranchInfo, UpstreamInfo, UpstreamCommit, BranchStatusInfo), fetch the data before deciding output mode, and serialize the gathered data (base branch details, upstream commit list and count, branch statuses, up_to_date and worktree conflict flags). The human-readable output path is preserved. replace emoji output with styled text for status messages Update human-facing terminal messages to use styled text instead of emoji glyphs. Replace "✅ Everything is up to date" with a bold green "Everything is up to date" string, replace the uncommitted-changes warning emoji line with a bold yellow "Warning: ..." message, and replace the "🔄 Updating branches..." line with a bold "Updating branches..." string. Split base.rs into base/mod.rs and base/json.rs Move JSON serde structs into a dedicated json.rs module to separate concerns and improve organization. The JSON output structs (BaseCheckOutput, BaseBranchInfo, UpstreamInfo, UpstreamCommit, BranchStatusInfo) were moved and made pub(super) so mod.rs can access them. The old base.rs file was removed. Use ANSI colors for 'but base check' output Replace emoji-based output with ANSI colored text for clearer, more professional terminal display. Base branch names are shown in cyan; the upstream count is yellow for >0 and green for 0. Commit SHAs are yellow with descriptions dimmed. Status indicators are now textual tags with color: [ok] green, [integrated] blue, [conflict] red/yellow, and [empty] dimmed. Headers are bold and labels dimmed for a cleaner visual hierarchy. This improves readability and removes distracting emoji icons. Fix upstream commit display in human output Human-readable output for the "Upstream commits" section incorrectly used recent_commits, causing it to show recent commits instead of actual upstream commits. Change the code to use upstream_commits for the human output so both JSON and human-readable views consistently show only true upstream commits. Use upstream_commits for base check JSON output Fix the JSON output for the "but base check" command to report only actual upstream commits. The code previously used recent_commits (which lists recent commits regardless of whether they're new upstream commits), causing the reported count to mismatch the listed commits. Switched to upstream_commits so the commits array matches the behind count and reflects only new upstream commits.
d150b14 to
4b29df3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| pub async fn handle( | ||
| cmd: Subcommands, | ||
| project: &LegacyProject, | ||
| out: &mut OutputChannel, | ||
| ) -> anyhow::Result<()> { |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function signature expects &LegacyProject but the caller in lib.rs passes &ctx (which is a &Context). This will cause a compilation error. The signature should be ctx: &Context instead of project: &LegacyProject, and throughout the function body, use ctx.legacy_project.id instead of project.id.
| project: &LegacyProject, | ||
| out: &mut OutputChannel, | ||
| ) -> anyhow::Result<()> { | ||
| match cmd { |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Fetch subcommand is defined in the args but is not handled in this match statement. This will cause a compilation error due to non-exhaustive pattern matching. Either add a handler for Subcommands::Fetch or explain why it was intentionally removed.
|
Moved to #11632 |
This is an update to the
but base checkcommand. Previously,--jsonwas a no-op, this actually implements it. It also previously showed recent commits rather than upstream commits. Finally, I updated the style to match the simpler, ansi colored style preferred by the rest of the commands, rather than the emoji laden on this command used.If
--jsonis specified, it now works properly: