From 03b2421e29cb4e044ff51354bef31773d252e6f9 Mon Sep 17 00:00:00 2001 From: L4XB Date: Tue, 15 Sep 2026 12:55:38 +0200 Subject: [PATCH] feat(cli): let every command write its result to a file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- CHANGELOG.md | 5 + src/sixsentences/cli.py | 145 ++++++++++++++++-------- tests/test_cli.py | 244 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 347 insertions(+), 47 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 413a5a2..f4998ef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,11 +12,16 @@ equivalents for Python package metadata. - `sixsentences --version` and `six-community --version` report the installed version of each command-line tool. +- Every `sixsentences` subcommand accepts `--output PATH`. A file receives + exactly the bytes stdout would have received, and is written only after the + command succeeds. ### Changed - The server package reads its version from the installed distribution metadata instead of a second literal in `sixsentences_server/__init__.py`. +- `sixsentences prisma --format svg --output PATH` now ends the file with a + newline, like every other written result. ## [0.2.0-alpha.1] - 2026-09-14 diff --git a/src/sixsentences/cli.py b/src/sixsentences/cli.py index 85b6bb0..8677b03 100644 --- a/src/sixsentences/cli.py +++ b/src/sixsentences/cli.py @@ -6,12 +6,12 @@ import json import os import sys -from collections.abc import Sequence +from collections.abc import Iterable, Sequence from dataclasses import asdict from pathlib import Path from typing import Any, cast -from pydantic import ValidationError +from pydantic import BaseModel, ValidationError from sixsentences import __version__ from sixsentences.connectors.openalex import OpenAlexClient, OpenAlexError @@ -55,36 +55,53 @@ def _load_jsonl(path: Path) -> list[WorkRecord]: return records -def _print_json(value: object) -> None: +def _json_text(value: object) -> str: if hasattr(value, "model_dump_json"): - print(value.model_dump_json(indent=2)) - return - print(json.dumps(value, ensure_ascii=False, indent=2, sort_keys=True)) + return cast(str, value.model_dump_json(indent=2)) + "\n" + return json.dumps(value, ensure_ascii=False, indent=2, sort_keys=True) + "\n" + + +def _jsonl_text(records: Iterable[BaseModel]) -> str: + return "".join(record.model_dump_json() + "\n" for record in records) + + +def _emit(args: argparse.Namespace, text: str) -> None: + """Write one complete result to `--output`, or to stdout. + A file receives exactly the bytes stdout would have received, so redirection + and `--output` cannot disagree about a trailing newline. The payload is + built in full before the path is touched, so a command that fails partway + leaves no half-written artifact behind. + """ -def _emit_records(records: Sequence[WorkRecord]) -> None: - for record in records: - print(record.model_dump_json()) + destination: Path | None = getattr(args, "output", None) + if destination is None: + sys.stdout.write(text) + return + destination.write_text(text, encoding="utf-8") def _command_query(args: argparse.Namespace) -> None: node = parse_query(args.query) if args.target == "display": - print(to_display(node)) + _emit(args, to_display(node) + "\n") elif args.target == "openalex": query, notes = compile_openalex(node) - _print_json( - { - "query": query, - "dropped_fields": notes.dropped_fields, - "dropped_wildcards": notes.dropped_wildcards, - } + _emit( + args, + _json_text( + { + "query": query, + "dropped_fields": notes.dropped_fields, + "dropped_wildcards": notes.dropped_wildcards, + } + ), ) elif args.target == "duckdb": sql, parameters = compile_duckdb(node) - _print_json({"sql": sql, "parameters": parameters}) + _emit(args, _json_text({"sql": sql, "parameters": parameters})) else: - print(translations(node)[args.target]) + _emit(args, translations(node)[args.target] + "\n") def _command_corpus_build(args: argparse.Namespace) -> None: @@ -93,13 +110,13 @@ def _command_corpus_build(args: argparse.Namespace) -> None: args.corpus, source=args.source, ) - _print_json(manifest) + _emit(args, _json_text(manifest)) def _command_corpus_search(args: argparse.Namespace) -> None: corpus = LocalCorpus(args.corpus) corpus.verify() - _emit_records(corpus.search(args.query, limit=args.limit)) + _emit(args, _jsonl_text(corpus.search(args.query, limit=args.limit))) def _command_openalex_search(args: argparse.Namespace) -> None: @@ -116,7 +133,7 @@ def _command_openalex_search(args: argparse.Namespace) -> None: ) finally: client.close() - _emit_records(records) + _emit(args, _jsonl_text(records)) def _command_rank(args: argparse.Namespace) -> None: @@ -126,8 +143,7 @@ def _command_rank(args: argparse.Namespace) -> None: protocol, now_year=args.now_year, ) - for item in ranked: - print(item.model_dump_json()) + _emit(args, _jsonl_text(ranked)) def _command_coverage(args: argparse.Namespace) -> None: @@ -135,42 +151,45 @@ def _command_coverage(args: argparse.Namespace) -> None: if not isinstance(payload, dict): raise ValueError("captures must be a JSON object mapping work ids to counts") counts = cast(dict[str, int], payload) - _print_json( - estimate_completeness( - counts, - args.occasions, - occasion_independence_verified=args.occasion_independence_verified, - ) + _emit( + args, + _json_text( + estimate_completeness( + counts, + args.occasions, + occasion_independence_verified=args.occasion_independence_verified, + ) + ), ) def _command_prisma(args: argparse.Namespace) -> None: counts = PrismaCounts.model_validate(_load_json(args.counts)) - output = render_flow_svg(counts) if args.format == "svg" else render_flow_text(counts) - if args.output is None: - print(output) - else: - args.output.write_text(output + ("\n" if args.format == "text" else ""), encoding="utf-8") + rendered = render_flow_svg(counts) if args.format == "svg" else render_flow_text(counts) + _emit(args, rendered + "\n") def _command_expansion_validate(args: argparse.Namespace) -> None: payload = _load_json(args.candidates) if not isinstance(payload, list) or not all(isinstance(item, str) for item in payload): raise ValueError("candidates must be a JSON array of query strings") - _print_json(validate_variants(args.existing, payload, limit=args.limit)) + _emit(args, _json_text(validate_variants(args.existing, payload, limit=args.limit))) def _command_data_profile(args: argparse.Namespace) -> None: dataset = parse_dataset_file(args.input) - _print_json( - { - "filename": dataset.filename, - "format": dataset.format, - "byte_count": dataset.byte_count, - "sha256": dataset.sha256, - "profile": asdict(dataset.profile), - "import_notes": dataset.import_notes, - } + _emit( + args, + _json_text( + { + "filename": dataset.filename, + "format": dataset.format, + "byte_count": dataset.byte_count, + "sha256": dataset.sha256, + "profile": asdict(dataset.profile), + "import_notes": dataset.import_notes, + } + ), ) @@ -197,7 +216,24 @@ def _command_data_analyze(args: argparse.Namespace) -> None: ) else: raise ValueError(f"unknown analysis recipe {args.analysis_kind!r}") - _print_json(asdict(analyze(dataset, recipe))) + _emit(args, _json_text(asdict(analyze(dataset, recipe)))) + + +def _add_output(parser: argparse.ArgumentParser, result: str, *, nested: bool = False) -> None: + """Give one command the shared `--output` option. + + `nested` suppresses the default on a recipe parser, so that accepting the + option in both positions does not let the recipe overwrite a value the + parent already read. + """ + + parser.add_argument( + "--output", + type=Path, + default=argparse.SUPPRESS if nested else None, + metavar="PATH", + help=f"write the {result} to PATH instead of stdout", + ) def _parser() -> argparse.ArgumentParser: @@ -222,18 +258,21 @@ def _parser() -> argparse.ArgumentParser: choices=("display", "openalex", "duckdb", "pubmed", "scopus", "wos", "ieee"), default="display", ) + _add_output(query, "compiled query") query.set_defaults(handler=_command_query) corpus_build = subcommands.add_parser("corpus-build", help="build a local corpus from JSONL") corpus_build.add_argument("input", type=Path) corpus_build.add_argument("corpus", type=Path) corpus_build.add_argument("--source", default="local-jsonl") + _add_output(corpus_build, "corpus manifest") corpus_build.set_defaults(handler=_command_corpus_build) corpus_search = subcommands.add_parser("corpus-search", help="search a local corpus") corpus_search.add_argument("corpus", type=Path) corpus_search.add_argument("query") corpus_search.add_argument("--limit", type=int, default=100) + _add_output(corpus_search, "matching records") corpus_search.set_defaults(handler=_command_corpus_search) openalex = subcommands.add_parser( @@ -245,12 +284,14 @@ def _parser() -> argparse.ArgumentParser: openalex.add_argument("--limit", type=int, default=200) openalex.add_argument("--year-from", type=int) openalex.add_argument("--year-to", type=int) + _add_output(openalex, "fetched records") openalex.set_defaults(handler=_command_openalex_search) rank = subcommands.add_parser("rank", help="rank JSONL works against a protocol") rank.add_argument("input", type=Path) rank.add_argument("protocol", type=Path) rank.add_argument("--now-year", type=int) + _add_output(rank, "ranked records") rank.set_defaults(handler=_command_rank) coverage = subcommands.add_parser("coverage", help="estimate search coverage with Chao2") @@ -264,12 +305,13 @@ def _parser() -> argparse.ArgumentParser: "without it the estimate is undetermined" ), ) + _add_output(coverage, "coverage estimate") coverage.set_defaults(handler=_command_coverage) prisma = subcommands.add_parser("prisma", help="render explicit PRISMA counters") prisma.add_argument("counts", type=Path) prisma.add_argument("--format", choices=("text", "svg"), default="text") - prisma.add_argument("--output", type=Path) + _add_output(prisma, "rendered flow") prisma.set_defaults(handler=_command_prisma) expansion = subcommands.add_parser( @@ -278,26 +320,32 @@ def _parser() -> argparse.ArgumentParser: expansion.add_argument("candidates", type=Path) expansion.add_argument("--existing", action="append", default=[]) expansion.add_argument("--limit", type=int, default=5) + _add_output(expansion, "validation result") expansion.set_defaults(handler=_command_expansion_validate) data_profile = subcommands.add_parser( "data-profile", help="profile a bounded CSV, TSV, JSON or XLSX dataset" ) data_profile.add_argument("input", type=Path) + _add_output(data_profile, "dataset profile") data_profile.set_defaults(handler=_command_data_profile) data_analyze = subcommands.add_parser( "data-analyze", help="run a deterministic analysis recipe over a bounded dataset" ) data_analyze.add_argument("input", type=Path) + # Accepted before the recipe and after it, because both read naturally. + _add_output(data_analyze, "analysis result") analysis_recipes = data_analyze.add_subparsers(dest="analysis_kind", required=True) - analysis_recipes.add_parser("missingness", help="count null and blank cells") + missingness = analysis_recipes.add_parser("missingness", help="count null and blank cells") + _add_output(missingness, "analysis result", nested=True) descriptive = analysis_recipes.add_parser( "descriptive", help="describe one complete numeric column" ) descriptive.add_argument("--column", required=True) + _add_output(descriptive, "analysis result", nested=True) group_summary = analysis_recipes.add_parser( "group-summary", help="aggregate complete numeric values by group" @@ -307,12 +355,14 @@ def _parser() -> argparse.ArgumentParser: group_summary.add_argument( "--metric", choices=("mean", "median", "sum", "count"), default="mean" ) + _add_output(group_summary, "analysis result", nested=True) correlation = analysis_recipes.add_parser( "correlation", help="compute complete-case Pearson correlation" ) correlation.add_argument("--x-column", required=True) correlation.add_argument("--y-column", required=True) + _add_output(correlation, "analysis result", nested=True) meta_analysis = analysis_recipes.add_parser( "meta-analysis", help="run DerSimonian-Laird random-effects pooling" @@ -320,6 +370,7 @@ def _parser() -> argparse.ArgumentParser: meta_analysis.add_argument("--effect-column", required=True) meta_analysis.add_argument("--se-column", required=True) meta_analysis.add_argument("--label-column") + _add_output(meta_analysis, "analysis result", nested=True) data_analyze.set_defaults(handler=_command_data_analyze) return parser diff --git a/tests/test_cli.py b/tests/test_cli.py index ad80d1d..88a7f4f 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -8,6 +8,7 @@ from sixsentences import __version__ from sixsentences.cli import main +from sixsentences.core.models import WorkRecord def test_query_command_compiles_openalex(capsys: object) -> None: @@ -111,3 +112,246 @@ def test_version_is_read_from_the_installed_distribution() -> None: declared = tomllib.loads(project.read_text(encoding="utf-8"))["project"]["version"] assert __version__ == declared + + +def _fixture_tree(root: Path) -> None: + """Write one synthetic input of every kind the CLI reads.""" + + root.mkdir(parents=True, exist_ok=True) + (root / "works.jsonl").write_text( + json.dumps({"id": "W1", "title": "Systematic evidence synthesis", "year": 2020}) + + "\n" + + json.dumps({"id": "W2", "title": "Evidence screening at scale", "year": 2021}) + + "\n", + encoding="utf-8", + ) + (root / "protocol.json").write_text( + json.dumps({"question": "Does screening scale?", "query_string": "evidence AND screening"}), + encoding="utf-8", + ) + (root / "captures.json").write_text(json.dumps({"W1": 2, "W2": 1}), encoding="utf-8") + (root / "counts.json").write_text( + json.dumps( + { + "records_identified": 10, + "duplicates_removed": 2, + "records_screened": 8, + "records_excluded": 5, + "records_unsure": 1, + "included": 2, + "reports_sought_for_retrieval": 3, + "reports_not_retrieved": 1, + "reports_assessed_for_eligibility": 2, + "reports_excluded_fulltext": 1, + "reports_included": 1, + "studies_included": 1, + } + ), + encoding="utf-8", + ) + (root / "candidates.json").write_text( + json.dumps(["evidence OR screening", "evidence AND appraisal"]), encoding="utf-8" + ) + (root / "observations.csv").write_text( + "group,score,other,effect,se\nA,2,4,0.3,0.1\nA,4,8,0.5,0.2\nB,8,16,0.4,0.15\n", + encoding="utf-8", + ) + + +def _deterministic_invocations(root: Path) -> dict[str, list[str]]: + """Every command whose result is identical on two consecutive runs.""" + + _fixture_tree(root) + corpus = root / "corpus" + assert main(["corpus-build", str(root / "works.jsonl"), str(corpus)]) == 0 + dataset = str(root / "observations.csv") + return { + "query-display": ["query", "evidence AND screening"], + "query-openalex": ["query", "evidence AND screening", "--target", "openalex"], + "query-duckdb": ["query", "evidence AND screening", "--target", "duckdb"], + "query-pubmed": ["query", "title:evidence AND screening", "--target", "pubmed"], + "corpus-search": ["corpus-search", str(corpus), "evidence"], + "rank": [ + "rank", + str(root / "works.jsonl"), + str(root / "protocol.json"), + "--now-year", + "2024", + ], + "coverage": ["coverage", str(root / "captures.json"), "--occasions", "2"], + "prisma-text": ["prisma", str(root / "counts.json")], + "prisma-svg": ["prisma", str(root / "counts.json"), "--format", "svg"], + "expansion-validate": ["expansion-validate", str(root / "candidates.json")], + "data-profile": ["data-profile", dataset], + "analyze-missingness": ["data-analyze", dataset, "missingness"], + "analyze-descriptive": ["data-analyze", dataset, "descriptive", "--column", "score"], + "analyze-group-summary": [ + "data-analyze", + dataset, + "group-summary", + "--group-by", + "group", + "--value-column", + "score", + ], + "analyze-correlation": [ + "data-analyze", + dataset, + "correlation", + "--x-column", + "score", + "--y-column", + "other", + ], + "analyze-meta-analysis": [ + "data-analyze", + dataset, + "meta-analysis", + "--effect-column", + "effect", + "--se-column", + "se", + ], + } + + +# Named here rather than derived, so parametrization needs no filesystem at +# collection time. `test_every_deterministic_command_is_parametrized` keeps the +# two lists in step. +_DETERMINISTIC = [ + "query-display", + "query-openalex", + "query-duckdb", + "query-pubmed", + "corpus-search", + "rank", + "coverage", + "prisma-text", + "prisma-svg", + "expansion-validate", + "data-profile", + "analyze-missingness", + "analyze-descriptive", + "analyze-group-summary", + "analyze-correlation", + "analyze-meta-analysis", +] + + +def test_every_deterministic_command_is_parametrized(tmp_path: Path) -> None: + """A command added to the fixture must not quietly go untested.""" + + assert sorted(_deterministic_invocations(tmp_path)) == sorted(_DETERMINISTIC) + + +@pytest.mark.parametrize("command", _DETERMINISTIC) +def test_output_writes_exactly_what_stdout_would_have_received( + command: str, tmp_path: Path, capsys: object +) -> None: + """Redirection and --output must never disagree about the result.""" + + argv = _deterministic_invocations(tmp_path)[command] + capsys.readouterr() # type: ignore[attr-defined] + + assert main(argv) == 0 + printed = capsys.readouterr().out # type: ignore[attr-defined] + + destination = tmp_path / "result.out" + assert main([*argv, "--output", str(destination)]) == 0 + written = capsys.readouterr() # type: ignore[attr-defined] + + assert destination.read_text(encoding="utf-8") == printed + assert written.out == "" + assert written.err == "" + + +def test_openalex_search_writes_its_records( + tmp_path: Path, capsys: object, monkeypatch: pytest.MonkeyPatch +) -> None: + """The one networked command uses the same writer, against a stub client.""" + + class StubClient: + def __init__(self, **_kwargs: object) -> None: + pass + + def search(self, _query: str, **_kwargs: object) -> list[WorkRecord]: + return [WorkRecord(id="W1", title="Evidence synthesis")] + + def close(self) -> None: + pass + + monkeypatch.setattr("sixsentences.cli.OpenAlexClient", StubClient) + destination = tmp_path / "records.jsonl" + + assert main(["openalex-search", "evidence", "--output", str(destination)]) == 0 + + assert capsys.readouterr().out == "" # type: ignore[attr-defined] + assert json.loads(destination.read_text(encoding="utf-8"))["id"] == "W1" + + +def test_corpus_build_writes_its_manifest(tmp_path: Path, capsys: object) -> None: + """Byte parity cannot be asserted here: the manifest carries a build time.""" + + _fixture_tree(tmp_path) + destination = tmp_path / "manifest.json" + + assert ( + main( + [ + "corpus-build", + str(tmp_path / "works.jsonl"), + str(tmp_path / "corpus"), + "--output", + str(destination), + ] + ) + == 0 + ) + + assert capsys.readouterr().out == "" # type: ignore[attr-defined] + manifest = json.loads(destination.read_text(encoding="utf-8")) + assert manifest["works"] == 2 + assert manifest["source"] == "local-jsonl" + + +def test_output_is_accepted_before_and_after_an_analysis_recipe( + tmp_path: Path, capsys: object +) -> None: + """A nested parser must not overwrite a value the parent already read.""" + + _fixture_tree(tmp_path) + dataset = str(tmp_path / "observations.csv") + before = tmp_path / "before.json" + after = tmp_path / "after.json" + + assert main(["data-analyze", dataset, "--output", str(before), "missingness"]) == 0 + assert main(["data-analyze", dataset, "missingness", "--output", str(after)]) == 0 + + assert capsys.readouterr().out == "" # type: ignore[attr-defined] + assert before.read_text(encoding="utf-8") == after.read_text(encoding="utf-8") + + +def test_a_failing_command_writes_no_file(tmp_path: Path, capsys: object) -> None: + """A partial result is worse than none; the path is touched only on success.""" + + _fixture_tree(tmp_path) + destination = tmp_path / "never-written.json" + (tmp_path / "observations.csv").write_text("score\nunknown\n", encoding="utf-8") + + assert ( + main( + [ + "data-analyze", + str(tmp_path / "observations.csv"), + "descriptive", + "--column", + "score", + "--output", + str(destination), + ] + ) + == 2 + ) + + assert not destination.exists() + assert "non-missing non-numeric" in capsys.readouterr().err # type: ignore[attr-defined]