fix(cli): <plugin> config set writes the value instead of printing - #9
Merged
Conversation
The plugin shim's `config` case forwarded everything to `cmd_config show` as trailing arguments, which ignores them. So `temper config set auto_nudge_lines 300` printed the config and exited 0 having set nothing, and `cairn config reset local` did the same. A silent success, in the CLI of a tool whose whole argument is that silent successes are the dangerous failure. The shim now forwards the subcommand and namespaces the key by looking up the section the manifest schema declares for it — which is the point of having a shim: you should not need to know that `trailers` lives in [git] rather than [cairn]. temper config set auto_nudge_lines 300 → [temper] auto_nudge_lines cairn config set trailers Signed-off-by → [git] trailers cairn config set pr.base develop → [cairn] pr.base, dots intact Prefixing is also what makes dotted keys work: _split_key splits on the first dot, so a bare `pr.base` would resolve to a section called `pr`. A key given already qualified is passed through, or `temper config set temper.auto_nudge_lines` would write temper.temper.auto_nudge_lines. get, unset and explain are namespaced the same way. doctor, path and edit pass through — they answer for the whole file, not a section. An unrecognised subcommand exits 1 and names the real ones. Trust re-hashing, scope parsing and the unknown-key warning all come free, because they live in cmd_config, which is now actually reached. Also: the `trailers` doc string suggested Co-Authored-By, and that text gets written into every config file the tool generates. 16 new assertions in tests/test_cli.sh, the regression first.
_plugin_section_for piped _schema_all into awk with `exit` after the first
match, closing the pipe while the writer was still going. Bash on Linux reports
the EPIPE as `printf: write error: Broken pipe` into whatever the caller
captured, so `temper config get` returned the value with an error message glued
to it.
This is the same bug the file already carries a warning about, three lines
above the function I added — "No `exit` in the awk". I wrote one anyway.
_schema_plugin_for's `!f { print; f = 1 }` is the pattern; use it.
Six stderr assertions added, following the precedent in test_config.sh: macOS
dies from SIGPIPE silently, so a value comparison does not reliably catch this
and it has to be asserted directly. That is why it passed locally and failed on
both CI platforms.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Found by the documentation audit in #8, and deliberately left out of it because it needs a behaviour change and a test rather than a doc edit.
The bug
Everything after
configwent toshowas trailing arguments, which ignores them. So:It printed the config and exited 0 having set nothing.
cairn config reset localdid the same. A silent success, in the CLI of a tool whose whole argument is that reporting success while doing nothing is the dangerous failure.Worth noting the sibling
*)case does reject unknown subcommands — onlyconfigswallowed.The fix
Forward the subcommand, and namespace the key by looking up the section the manifest schema declares for it. That lookup is the point of having a shim: you should not need to know that
trailerslives in[git]rather than[cairn]._schema_allalready emitssection|key|plugin, so_plugin_section_foris a lookup rather than a second place to encode which key lives where.Three details that decide whether this is correct, each with an assertion:
_split_keysplits on the first dot, so a barepr.basewould resolve to a section calledpr. There is a test that no[pr]section is invented.temper config set temper.auto_nudge_lines 300writestemper.temper.auto_nudge_lines.doctor,pathandeditare not section-scoped, so they forward unchanged.Everything else comes free — trust re-hashing,
global/localscope parsing, the unknown-key warning — because it lives incmd_config, which is now actually reached.An unrecognised subcommand exits 1 and names the real ones.
Also
The
trailersdoc string suggestedCo-Authored-By. That string is written as a comment into every config file the tool generates, so it was quietly recommending the one trailer this project's own rules forbid. NowSigned-off-by.Verification
16 new assertions in
tests/test_cli.sh, the regression first:726 assertions across 10 files pass.
Docs updated in the same change: the three plugin READMEs said "the plugin shim's
configonly shows", which was true when written and is not now, and the CLI reference gains the equivalence.