Add --only-commit flag to stellar version#2592
Conversation
|
Claude finished @leighmcculloch's task in 1m 19s —— View job Claude PR Review
Small, well-scoped change. The new flag mirrors the existing Findings1.
2. Empty-string output when
3. Mutually exclusive flags fail silently (nit, pre-existing)
Other notes
Overall: LGTM once |
There was a problem hiding this comment.
Pull request overview
Adds a --only-commit output mode to the stellar version command so scripts can retrieve the embedded git SHA without parsing the multi-line version output.
Changes:
- Introduces
--only-commitflag to print just the build’s commit SHA. - Updates
stellar versiondispatch logic to support the new output mode.
|
Claude finished @leighmcculloch's task in 1m 49s —— View job Claude PR Review
The follow-up commit Findings1. Doc regen produced a much larger diff than expected — verify it's intentional (medium / worth a quick sanity check)
That's fine, but two things worth confirming before merge:
2. Mutually-exclusive flags still resolved by
3. No test for
#[test]
fn version_only_commit() {
let sandbox = TestEnv::default();
sandbox
.new_assert_cmd("version")
.arg("--only-commit")
.assert()
.success()
.stdout(format!("{}\n", soroban_cli::commands::version::git()));
}Consistent with the lack of coverage for 4. Empty-string output when
Fix #2 (add ArgGroup) → • Fix #3 (add test) → SummaryLGTM. The feature itself is correct and matches the established pattern; the previous blocker (regenerating |
|
Claude finished @leighmcculloch's task in 1m 28s —— View job Claude PR Review
The latest push ( That fully resolves my previous concern about the +2314/-1811 reflow in Findings1. Mutually-exclusive flags still resolved by
2. No test for
#[test]
fn version_only_commit() {
let sandbox = TestEnv::default();
sandbox
.new_assert_cmd("version")
.arg("--only-commit")
.assert()
.success()
.stdout(format!("{}\n", soroban_cli::commands::version::git()));
}Consistent with the lack of coverage for the sibling flags, so not blocking — but if you add one, adding the trio would be cheap. 3. Empty-string output when
SummaryLGTM. The previously-flagged doc-diff concern is resolved; remaining items (ArgGroup, tests) are nice-to-have and consistent with the surrounding code's style. Net diff is minimal and focused. |
|
Claude finished @leighmcculloch's task in 1m 17s —— View job Claude PR Review
The new commit Net PR diff against Small, focused, and correct. Findings1. No test for
The second one is the more valuable test, since the #[test]
fn version_only_commit() {
let sandbox = TestEnv::default();
sandbox
.new_assert_cmd("version")
.arg("--only-commit")
.assert()
.success()
.stdout(format!("{}\n", soroban_cli::commands::version::git()));
}
#[test]
fn version_only_flags_are_mutually_exclusive() {
let sandbox = TestEnv::default();
sandbox
.new_assert_cmd("version")
.args(["--only-version", "--only-commit"])
.assert()
.failure();
}2. Empty-string output when
SummaryLGTM. All previously-raised review items either resolved ( |
What
Add an
--only-commitflag tostellar versionthat prints just the commit sha of the build, alongside the existing--only-versionand--only-version-majorflags.Why
The commit sha is already embedded in the build and shown as part of the full version output, but there is no way to extract it on its own. A dedicated flag makes the commit sha consumable by scripts and tooling without needing to parse the multi-line full version output.
Known limitations
N/A