Skip to content

fix: normalize version command output - #46

Merged
chaim0m merged 3 commits into
mainfrom
codex/dci-version-output
Aug 4, 2026
Merged

fix: normalize version command output#46
chaim0m merged 3 commits into
mainfrom
codex/dci-version-output

Conversation

@chaim0m

@chaim0m chaim0m commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • make dci version use the same <binary> version <version> format as dci --version
  • write through Cobra OutOrStdout()
  • verify the command uses its configured root name rather than a tautological production value

Why

Follow-up to Alfredo's review comment on #36. The two version entry points exposed different formats.

Test methods

go test ./...
go vet ./...
go build -o /tmp/dci-version-output-test .
/tmp/dci-version-output-test version
/tmp/dci-version-output-test --version

Could this break things?

Risk: zero for released users. The bare dci version format was introduced after v1.5.1 and has never shipped. Networking, authentication, and API commands are unchanged.

Jira

CMP-48648

@chaim0m
chaim0m requested a review from apgiorgi as a code owner August 3, 2026 17:20
@chaim0m
chaim0m requested a review from taltultc August 3, 2026 17:20
@chaim0m chaim0m self-assigned this Aug 4, 2026

@apgiorgi apgiorgi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct change, and it does what I asked for in the #36 review. Verified the target format is right: cobra v1.6.1's default version template is {{with .Name}}{{printf "%s " .}}{{end}}version {{.Version}} and restish sets root Use: filepath.Base(os.Args[0]), so both dci version and dci --version now emit dci version <v>. Also confirmed this isn't superseded by the recent work on main — 635da95/13f0081/1e75fef touched normalizeArgs and the help path, not this line — and git merge-tree produces a clean tree.

Nits only:

1. The PR body invents a compat risk. It says "scripts parsing the previous bare dci version value must accept the same standard format." dci version was introduced in 635da95, which is after v1.5.1 (git rev-list -1 v1.5.1 is 6b4de08, and git tag --contains 635da95 is empty). The bare format never shipped, so the risk is zero, not "low-medium".

2. Prefix. fix: puts a changelog entry about normalizing output for a command that's never been released. chore: fits better per AGENTS.md, or just squash it into 635da95.

3. My premise in the #36 comment was wrong and this inherits it. I said surrounding registrations use cmd.OutOrStdout()/cli.Stdout. They don't — every local command writes bare os.Stdout (main.go:966, 979, 990, 1060, 1090-1108, 1296). This makes version the only one using OutOrStdout(). Harmless in production (restish never calls Root.SetOut outside its own tests, so it resolves to os.Stdout) and it is the more testable pattern, so keep it — but it doesn't remove an inconsistency the way I implied, and the rest of the file is still worth a sweep sometime.

4. The test is partly tautological (main_test.go:124-128). want recomputes cli.Root.Name() the same way the code does, and setupTestRoot hardcodes Use: "dci", so the production filepath.Base(os.Args[0]) path is untested. The format literal is verified; the name source isn't.

5. Duplicating cobra's "%s version %s" literal will drift silently if the default version template changes on a cobra bump. Not worth restructuring for, just noting it.

@chaim0m
chaim0m requested a review from a team as a code owner August 4, 2026 12:11
@chaim0m
chaim0m requested a review from apgiorgi August 4, 2026 12:16
@chaim0m

chaim0m commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator Author

Updated the PR risk to zero for released users and replaced the tautological root-name assertion with a custom configured command name. go test ./... and go vet ./... pass. I retained OutOrStdout() because it is the more testable Cobra behavior. I did not rewrite the already-published commit solely for the prefix, since that would require a force-push.

@chaim0m
chaim0m merged commit d2b5884 into main Aug 4, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants