Repository navigation
internal/cmd: Cobra root, App, persistent flags, version - #52
Conversation
`labelsync version` has an answer, and it is one value. Info would put it on stderr, where a shell substitution cannot see it; a one-row bordered table would reach stdout by pretending the value is something it is not. The output page already said this case would get a new product-level method rather than moving Info back to stdout — this is that method. WriteResult takes both projections at once and gives each audience only its own: the pretty writer renders the format string, the JSON writer marshals the record and drops the prose. That keeps the NDJSON invariant intact — every line on stdout is still one typed object, never a sentence — and it makes --dont-prettify a choice of phrasing rather than a second output path, since JSON has no prose in it to strip. It is deliberately not a general-purpose print: the doc comment and the review checklist both say to reach for it only when the command's whole answer is one value. Rows still go through output.Table, narration still goes through Info. The Sprintf is assigned to a variable rather than nested in the Fprintln call. The nested form is what fmt.Fprintf exists for and staticcheck says so, but Fprintf is not on the errcheck exclusion list and output is best-effort.
The skeleton every later command hangs off, so subsequent commands add leaves
rather than inventing their own wiring.
root.go builds the root, defines the seven persistent flags, and resolves them
once in PersistentPreRunE. app.go holds the App those commands close over: the
output writer, the handle on the debug log level, and the resolved flag values.
version.go owns Version — the variable .goreleaser.yml and the Dockerfile inject
by full path, which is why it has to keep exactly that name in exactly this
package, and why version_test.go asserts both build files still name it. That
path is a string the compiler never checks; a rename would ship every release as
"dev" with nothing failing.
The wiring is the part worth reviewing, and all of it is in
docs/.../architecture/output.md § Wiring it in Cobra:
- Writers and the logger come from cmd.OutOrStdout()/cmd.ErrOrStderr(), never
the NewDefault* constructors. In production they resolve to the same files;
under test they are the buffers, which is the only reason a command test can
assert on output at all. NewApp still builds a default writer, for the
window before flags are parsed where a parse failure needs somewhere to go.
- An unrecognised --output is wired as pretty before it is rejected. The
rejection has to have somewhere to go too.
- SilenceUsage, or every runtime failure drags the usage block along behind
it. SilenceErrors, or Cobra prints its own copy of the message in addition
to main's, and its copy carries no error_kind.
- os.Exit lives in main and nowhere else: from inside a command it would skip
deferred cleanup and leak temp files, unreleased locks, and unflushed
writers. Commands return errors; exit.Of turns them into codes.
main's error handling moved into a report() function, because os.Exit cannot be
tested and everything above it can. The silence guard follows the carrier's Err
field, not its Code: a carrier holding a real failure prints even when its code
is outcome-shaped, and only a nil Err means the non-zero code was itself the
answer.
Invalid flag values return plain errors rather than new sentinels. The sentinels
describe how a run can fail once it is under way; a value rejected before any
work starts is a usage error, and adding to the error_kind contract is not
something to do by reflex.
Docs: the CLI wiring section of the output page rewritten now that it describes
code rather than a plan, a new section for WriteResult, the overview page given
a "How the tree is wired" section, and the design plan marked partly landed. The
overview's exit-code table still said Skipped was 3; corrected to 4 to match
what shipped. README written out from its one-line stub — global flags, the
version command, the output contract, and the exit codes, including that the
outcome codes are bits and callers should test them as bits.
|
Pushed Both spellings call the same The flag is hand-rolled rather than Cobra's built-in It is a local flag, not a persistent one — it answers for the binary, and Giving the root a On the version string itselfThe bare SHA is not a bug, and the recipe is identical to Documented in the README so the next person does not read a SHA as a broken build:
Worth noting the one asymmetry, inherited from |
|
Two more pushes, both about where things are written down rather than what the code does.
The one subtlety in the build recipe: the fallback is captured before the strip. Piping git into
Rebased twice: the two README commits are now |
9dece1c to
333e123
Compare
Both go through writeVersion, so the two spellings cannot drift into disagreeing about what the version looks like. The flag is hand-rolled rather than enabled with Cobra's built-in cmd.Version + SetVersionTemplate, because Cobra handles that flag inside execute(), before PersistentPreRunE: at that point --output has not been read and app.Out is still the pre-parse fallback writer, so `--output=json --version` would print a bare line into a stream that is supposed to be typed JSON objects, on os.Stdout where a test could not see it. Routing it through the root's RunE gets it the writer the user asked for. It is a local flag, not a persistent one. It answers for the binary, and `labelsync sync --version` is not a question sync should have an opinion about — which is also where Cobra puts its own. Giving the root a RunE changes what a bare `labelsync` runs, so two tests pin the behaviour that must not change with it: no flag still prints the help, and an unknown subcommand still fails rather than quietly showing it.
The Output page cited a sibling codebase by name five times: once for the Info-on-stdout divergence, and four times in the pitfalls section. The reasoning is what those passages are for, and every one of them stands on its own without the attribution — a default log level that nothing happens to use is a defect whoever wrote it. Naming it also dates badly. A reader who cannot see that repository learns nothing from the name, and a defect fixed over there quietly turns this page into a false claim about someone else's code. The provenance itself is worth keeping, and it stays where it belongs: one line in AGENTS.md and the comparisons in the design plan, which is explicitly a document about what this design borrows and from where. Drops the link to the sibling repository's issue tracker with it. The receipt is worth less than a page that reads as being about labelsync.
goreleaser injects {{ .Version }}, the tag with the leading v already stripped,
so a released binary said 1.2.3 while `task build` on that same tag said v1.2.3.
Two spellings of one version, and no way to tell from the string which build
produced it.
`task build` now strips the v too. The fallback is captured before the strip
rather than after, because piping git through the strip would swallow its exit
status and hand the build an empty version instead of "dev" — a build that
reports nothing at all is worse than one that reports the wrong thing. Nothing
else needs guarding: a describe fallback is a hex SHA, which cannot start with a
v.
A README should say what the tool is, how to install it, and where the documentation is. This one had grown a global-flag table, the config search order, the NDJSON shapes, the exit-code contract, and a table of what version string each build produces — reference material that a reader has to scroll past to find out whether labelsync is the thing they want, and that now has to be kept in step with the docs it duplicates. Two destinations. docs/content/docs/usage/ is new, and is where the user-facing reference goes: the commands, the global flags, and where the config file is found. It is deliberately outside the architecture section, which says on its first line that it holds internal design documents — a flag table is not one. The versioning table gets its own architecture page rather than a paragraph in an existing one, because it is about the build rather than the code: the injected variable, what each build produces, and why the local recipe strips the leading v and captures its fallback before doing so. What is left is a description, a status notice, install, a table of where to read more, and how to run the checks. The notice says only that this is a work in progress — the version that listed which subsystems had landed was a second changelog to keep honest, stale the moment a command lands without someone remembering to edit it. #47 carries the box to drop it once a release exists. AGENTS.md's convention is updated to match, since it is what a change gets held to at review: user-facing changes go to docs/content/docs/usage/, and the README is touched only when what labelsync is or how it is installed changes.
333e123 to
52e5245
Compare
1 participant
Closes #14.
The command tree's skeleton —
root.go,app.go,version.go— so subsequent command issues addleaves rather than inventing their own wiring.
task lint,task test, andtask md:checkallpass.
What landed
root.gobuilds the root, defines the seven persistent flags, and resolves them once inPersistentPreRunE.app.goholds theAppevery command closes over: the output writer, thehandle on the debug log level, and the resolved flag values.
version.goownsVersion.The wiring follows Output & Exit Codes § Wiring it in Cobra
exactly: writers and the logger from
cmd.OutOrStdout()/cmd.ErrOrStderr(), bothSilence*flags set, and
os.Exitonly inmain.Three decisions worth reviewing
WritergainedWriteResult.versionhas an answer and it is one value, not a table.Infowould put it on stderr where
$(labelsync version --dont-prettify)cannot see it; a one-row borderedtable would reach stdout by pretending the value is something it is not. The output page already
said this case would get a new product-level method rather than moving
Infoback to stdout, sothis is that method rather than a new decision. It takes both projections at once — pretty renders
the format string, JSON marshals the record and drops the prose — which keeps every stdout line one
typed object and makes
--dont-prettifya choice of phrasing rather than a second output path.main's error handling is areport()function, not four inline lines.os.Exitcannot betested and everything above it can.
main_test.godrives it over nil, a plain failure, a carrierholding a failure, and a silent carrier. The silence guard follows the carrier's
Errfield ratherthan its
Code: a carrier holding a real failure still prints even when its code is outcome-shaped.Invalid flag values return plain errors, not new sentinels.
--output yaml,--concurrency 0,--write-rate -1, and a negative--max-waitare rejected with a plainfmt.Errorf, whichexit.Ofmaps to1. The sentinels describe how a run can fail once it is under way; a valuerejected before any work starts is a usage error, and
error_kindis a public contract not worthadding to by reflex. Happy to add one if you disagree — it is three lines plus a doc row and a test
row.
Tests
internal/cmdis driven the waymaindrives it — build the tree,SetOut/SetErrat buffers,execute — so a test that cannot see the output is the failure signal for bad wiring.
--output=json/-o jsonselects the JSON writer, asserted by parsing what a command actuallyproduced;
exit.Ofreturning1;ErrRepoInaccessiblekeeps its sentinel out through the tree, soKindOfstillyields
repo_inaccessible;SilenceUsage/SilenceErrorsasserted as fields and behaviourally — after a failing commandneither stream carries a usage block or Cobra's own copy of the message;
--debuggatesslog, on the command's stderr and never on stdout;versionin all three renderings, plus a test that.goreleaser.ymland theDockerfilestillname
github.com/specsnl/labelsync/internal/cmd.Version. That path is a build-file string thecompiler never checks; a rename would ship every release as
devwith nothing failing.Docs & README
The output page's Cobra section rewritten now that it describes code rather than a plan, plus a new
section for
WriteResult. The overview page gained a "How the tree is wired" section, and itsexit-code table said
Skippedwas3— corrected to4to match what shipped in #51. The designplan's § CLI is marked partly landed.
The README was a one-line stub; #13 deferred the exit codes and the
--outputcontract to it. Itnow covers the global flags,
version, the stdout/stderr split with the NDJSON shapes, and the exitcodes — including that the outcome codes are bits and callers should test them as bits.
Scope
Every box on #14 is ticked. Nothing left unticked.