fix: preflight optional YAML output before reconciliation - #34
Conversation
| ) -> str: | ||
| """Reject unavailable optional renderers before command side effects.""" | ||
|
|
||
| if value.lower() == "yaml" and find_spec("yaml") is None: |
There was a problem hiding this comment.
Correctness (plausible): find_spec("yaml") is None only checks that a module spec is discoverable, not that import yaml will actually succeed. A partially-installed / broken PyYAML (or a same-named shadowing module) would pass this preflight, so release reconcile --format yaml still persists last-reconciliation.json before render_records -> require_yaml's real import yaml fails later — reproducing the exact persist-then-crash race this PR sets out to fix.
Flagged by automated review (base-cli-demo#34).
| result = invoke(["release", "reconcile", "--format", "yaml"], tmp_path) | ||
|
|
||
| assert result.exit_code == 1 | ||
| assert "base-cli-demo[yaml]" in result.output |
There was a problem hiding this comment.
Test robustness (plausible): this asserts the ClickException text via result.output, but base_cli.testing.invoke forces CliRunner(mix_stderr=False) whenever that constructor parameter exists (Click < 8.2, still permitted by pyproject.toml's click>=8.1,<9). Under that config .output reflects stdout only and the exception text lands in .stderr instead, so this assertion would fail if dependency resolution ever lands on Click 8.1.x. It only passes today because CI/dev environments resolve the newest Click 8.4.x, where .output unconditionally mixes both streams.
Flagged by automated review (base-cli-demo#34).
| ) | ||
|
|
||
|
|
||
| def _check_format_dependency( |
There was a problem hiding this comment.
Altitude: the consumer re-implements "is the optional YAML renderer available" via find_spec instead of reusing base_cli's own check, letting the two diverge (see the find_spec-vs-import gap flagged on line 28). base_cli only exposes this as the private base_cli._dependencies.require_yaml (a real try/import, not find_spec) — there's no public preflight helper, so every consumer with a --format option has to hand-roll an equivalent, and a future optional format would need the same ad hoc logic copy-pasted again instead of one shared, correct base_cli entry point.
Flagged by automated review (base-cli-demo#34).
|
Consistency gap (src/base_cli_demo/rich_scenario.py, around line 22): this file is outside the diff so it can't take an inline comment, but it's directly relevant to this PR's goal. Flagged by automated review (base-cli-demo#34). |
1 participant
Fixes #21