feat(cli): let every command write its result to a file - #147
Conversation
|
I have read and agree to the SixSentences CLA v1.0. |
434418f to
c9c6d6b
Compare
Self-reviewRecorded under the founding-maintainer exception in
The judgement call
ChecksEvery required context is green after the rebase. Engine only; the API, web Merging. |
c9c6d6b to
356aea2
Compare
Exactly one of fifteen subcommands took `--output`. The rest printed to stdout and nothing else, so keeping a compiled query for a methods appendix or a coverage estimate for a reproducibility record meant shell redirection — which captures whatever was printed and gives no signal when the command failed partway through. Every command now takes it, because every command in this CLI prints a result rather than a progress report. `corpus-build` is the closest call: it does work and then reports on it, but what it reports is the checksummed manifest, which is exactly the artifact a reproducible build needs to keep. Two decisions worth naming. A file receives exactly the bytes stdout would have received. `_command_prisma` used to append a newline for text and not for SVG, so the same command wrote different bytes to a file than to a pipe. One rule replaces that: build the complete payload, then either write it or print it. The only visible consequence is that a written SVG now ends with a newline. The path is touched only once the payload is complete, so a command that raises partway leaves no half-written artifact — the failure mode redirection cannot avoid. `data-analyze` accepts the option before the recipe and after it. A nested parser normally clobbers a value the parent already read, because argparse copies the whole sub-namespace back; `argparse.SUPPRESS` as the recipe default keeps the parent's value when the recipe does not set one. Closes #124 Signed-off-by: L4XB <L4XB@users.noreply.github.com>
356aea2 to
03b2421
Compare
Addendum to the self-reviewRebased twice since that comment was written — once onto #148, once onto #151 — The All 23 contexts green. Merging. |
1 participant
Summary
Exactly one of fifteen subcommands took
--output. Every command takes it now,and they all behave the same way, because they all go through one writer.
Which commands were left out, and why
None. The issue asks to exclude commands whose output is a progress report
rather than a result; applying that test to this CLI leaves an empty set. Each
of the fifteen prints one artifact and then exits:
querycorpus-buildcorpus-searchopenalex-searchrankcoverageprismaexpansion-validatedata-profiledata-analyze× 5 recipescorpus-buildis the one that could be argued either way: it performs work andthen reports on it. But the report is the manifest — source, work count, and
the SHA-256 of the snapshot — which is precisely what a reproducible build has
to keep. Progress, where it exists at all, goes to stderr and never enters the
file.
Behavior and compatibility
A file gets exactly the bytes stdout would have got
_command_prismaappended a trailing newline for--format textand not for--format svg, so the same command wrote different bytes to a file than to apipe. That asymmetry was the only precedent to copy, and copying it fifteen
times would have made
--outputmean something slightly different per command.One rule replaces it: build the complete payload — trailing newline included,
exactly as
printwould have produced it — then either write it or print it.diff <(sixsentences … ) fileis empty for every command.One compatibility note:
sixsentences prisma counts.json --format svg --output flow.svgnow writes a file ending in\n. One byte, on a formatwhere trailing whitespace outside the root element is insignificant, in
exchange for one rule instead of a per-command exception. Called out here
because it is a change to an existing command rather than a new option.
A failed command writes nothing
The payload is assembled in full before the path is opened, so a command that
raises partway leaves no artifact — not a truncated one, and not a stale one it
started to overwrite. This is the failure mode the issue names about
>, whichcreates the file before the command has produced anything.
test_a_failing_command_writes_no_filepins it.data-analyzetakes it in both positionsBoth work and produce the same file. This needed care:
_SubParsersActionparses the recipe into a fresh namespace and then copies every key back over
the parent's, so a recipe-level
--outputwith the usualdefault=Nonewouldsilently erase a value the parent had already read.
argparse.SUPPRESSas therecipe default leaves the key absent unless it was actually given.
test_output_is_accepted_before_and_after_an_analysis_recipepins it.Not changed
--outputis absent: byte-for-byte identical. The pre-existingtests in
tests/test_cli.pywere not touched and still pass.Validation
The 21 new tests are:
byte for byte — one per deterministic command, including both
prismaformats and all five analysis recipes;
openalex-searchagainst a stub client, so the networked command is coveredwithout egress;
corpus-build, where byte parity cannot be asserted becauseCorpusManifest.created_atcarries a build time — the test reads the writtenmanifest instead;
Engine only. The API, web client, browser extension, macOS Companion and the
self-hosting definition do not call this CLI.
Review boundaries
--outputwrites to a paththe caller supplied on their own command line, with the caller's own
permissions, using
Path.write_text. No path is derived from input data,no directory is created, and an existing file is replaced rather than
appended to. Authentication, tenancy, uploads, retention and deletion are
not involved.
openalex-searchreaches the network exactly as beforeno research-facing behavior changed. Each result keeps its stated limits;
writing it to a file makes the record easier to keep beside the data it
describes, not easier to strip.
Source-release hygiene
CHANGELOG.mdis updated for user-visible changes.Signed-off-bytrailer.CLA.mdand posted its exact acceptance sentence as a standalone pull-request comment.Visual evidence
No UI change.
Noticed while here, not fixed here
corpus-buildcannot produce a byte-identical result twice:CorpusManifest.created_atdefaults toutcnow(). The snapshot itself isdeterministic and checksummed; only the manifest wrapper carries the time. It is
worth deciding whether that field belongs in a document whose point is to be
comparable — but not in this pull request.
Closes #124