Wire the PDF output, and stop ignoring ReportConfig - #28
Merged
Conversation
Closes #21. `ReportConfig.formats` was typed to accept only json and summary, the CLI never imported either PDF renderer, and `run --help` said nothing about PDF. So 354 lines of renderer — 282 statements at 91% coverage, 72 at 90% — could not be produced by any invocation of the shipped tool, while the README advertised `pip install "markproof[pdf]"` as though installing the extra got you one. The PDF is the artefact you hand to an auditor: the one output whose entire purpose is to be read by someone who will never run the tool. Having it exist only in the source tree was the gap least likely to be noticed and most likely to matter. `formats` now accepts `pdf` and `pdf-html`, and the renderer is imported at the point of use rather than at module scope — the default output path must never depend on a package the base install does not ship. When a renderer is unavailable, its own message is passed through: WeasyPrint's covers the case that actually bites people, where the wheel is installed but ctypes cannot find Pango or cairo, and nothing reconstructed here would say it better. Two neighbouring things the audit found, fixed with it: `report.sign_key` was validated and then ignored, so an operator who wrote `sign_key: env:CI_SIGNING_KEY` got an unsigned report and no hint why. MARKPROOF_SIGNING_KEY stays the fallback, because that is what CI documentation everywhere assumes. `report.output_dir` was likewise ignored. Asking for a PDF in the config and getting nothing because `--report-dir` was not also passed would discard the request silently, so a non-default format is now enough on its own. Writing the report was also outside the guarded block — the same defect as #22 one layer further on. A missing extra propagated as a traceback and exit 1, which in this tool means "a rule failed". It is exit 2 now, with a sentence. Six CLI tests, including that a real PDF is written (`%PDF-` and two pages), that asking for `[json]` alone does not also write a summary, and that a missing renderer produces the install command rather than a stack trace. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The pytest jobs installed only the dev extra, so the newly wired PDF output was untested on every runner — the same shape of gap as the one this PR closes. CI now installs [pdf] as well, and a contributor without it gets a skip rather than a puzzling failure. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
Closes #21. Sixth step of the audit remediation — and the last item in step 3 of
the plan.
The gap
354 lines of PDF renderer, 91% and 90% covered, unreachable:
ReportConfig.formatswasLiteral["json", "summary"]cli.pyimported neither renderermarkproof run --helpmentioned no PDF…while the README said "PDF is opt-in (
pip install "markproof[pdf]")", whichreads as install the extra and you get PDFs. You did not.
The PDF is the artefact you hand an auditor — the one output whose whole purpose
is to be read by someone who will never run the tool. Existing only in the source
tree made it the gap least likely to be noticed and most likely to matter.
Wired
Imported at the point of use, never at module scope: the default output path must
not depend on a package the base install does not ship. When a renderer is
unavailable, its own message is passed through — WeasyPrint's covers the case
that actually bites people, where the wheel is installed but ctypes cannot find
Pango or cairo, and nothing reconstructed in the CLI would say it better.
Two things it was sitting next to
report.sign_keywas validated and then ignored. An operator writingsign_key: env:CI_SIGNING_KEYgot an unsigned report and no hint why.MARKPROOF_SIGNING_KEYstays the fallback, because that is what CI documentationeverywhere assumes.
report.output_dirwas ignored too. Asking for a PDF in the config andgetting nothing because
--report-dirwas not also passed would discard therequest silently, so a non-default format is now sufficient on its own.
And the same bug as #22, one layer further on
Report writing sat outside the guarded block, so a missing extra propagated as a
traceback and exit 1 — which in this tool means a rule failed. Exit 2 now,
with a sentence. Worth noting that #22's fix did not generalise on its own; this
needed finding separately, which is what the test for it now prevents.
Tests
Six, including: a real PDF is written (
%PDF-1.4, two pages);formats: [json]alone does not also write a summary — proving the field is honoured rather
than decorative; and an unavailable renderer yields the install command, not a
stack trace.
437 tests, ruff + mypy strict clean.
🤖 Generated with Claude Code