fix: normalize version command output - #46
Conversation
apgiorgi
left a comment
There was a problem hiding this comment.
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.
|
Updated the PR risk to zero for released users and replaced the tautological root-name assertion with a custom configured command name. |
Summary
dci versionuse the same<binary> version <version>format asdci --versionOutOrStdout()Why
Follow-up to Alfredo's review comment on #36. The two version entry points exposed different formats.
Test methods
Could this break things?
Risk: zero for released users. The bare
dci versionformat was introduced after v1.5.1 and has never shipped. Networking, authentication, and API commands are unchanged.Jira
CMP-48648