Wire up PDF output (port of upstream PR #367)#8
Conversation
|
@tobiasfalk — heads up, your upstream #367 (Complete PDF output handler) is being incorporated here. Context: We use WireViz to document Classic Mini Cooper wiring harnesses at Classic Mini DIY and are building a GUI on top of it. This is your second PR we've pulled in (after #379 The port uses the in-memory Commit attribution links back to your PR. Thanks! |
There was a problem hiding this comment.
Code Review
This pull request implements PDF output support by enabling the 'P' flag in the CLI and adding the rendering logic in the Harness class. A review comment notes that the current implementation only renders the Graphviz diagram, which contradicts documentation stating that PDF output should also include the Bill of Materials (BOM); it is suggested to either update the documentation or extend the PDF generation to include the BOM.
| if "pdf" in fmt: | ||
| outputs["pdf"] = graph.pipe(format="pdf") |
There was a problem hiding this comment.
The current implementation of PDF output only renders the Graphviz diagram. However, the docstring in src/wireviz/wireviz.py (line 55) states that the PDF format includes "the diagram and (depending on the template) the BOM".
Since this implementation only pipes the graph to PDF, the resulting file will miss the summary Bill of Materials (BOM) table that is present in the HTML output. To maintain consistency with the documentation, you should either update the docstring in wireviz.py to clarify that PDF only contains the diagram (consistent with PNG and SVG), or consider extending the implementation to include the BOM.
Implements the ``pdf`` output format that's been a TODO stub since
v0.4.1. Pipes the graph through Graphviz's PDF renderer
(``graph.pipe(format="pdf")``) and dispatches the bytes the same way
PNG goes — to a file in normal mode, to ``sys.stdout.buffer`` in
stdout mode.
Usage:
wireviz -f P harness.yml # produces harness.pdf
cat harness.yml | wireviz -f P -O - - # stdout, binary
CLI flag changes:
* ``"P": "pdf"`` un-commented in ``format_codes`` (use ``-f P``)
* "PDF output is not yet supported" stderr warning removed from
Harness.output()
Adapted from wireviz#367 (originally
by @tobiasfalk, targeting upstream ``dev``). The upstream patch went
through the old ``graph.render()`` + temp-file path — this port uses
the in-memory ``graph.pipe()`` wired up by the stdin/stdout refactor
(PR wireviz#321), so PDF works in both file mode AND stdout mode without
extra plumbing.
Verified:
* ``wireviz -f P harness.yml`` produces a valid PDF (file 1.7).
* ``cat harness.yml | wireviz -f P -O - -`` writes valid PDF to stdout.
* build_examples.py: deterministic outputs (.gv, .bom.tsv) byte-
identical (no example .yml has been switched to request PDF
rendering — keeping that out of the regression baseline since PDF
bytes from graphviz vary by version).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…eedback) Addresses gemini-code-assist feedback on #8 — the prior docstring claimed PDF includes "the diagram and (depending on the template) the BOM", which was carried over from upstream's never-completed PDF stub plan. The actual implementation in this PR pipes the graph through Graphviz's PDF renderer (graph.pipe(format="pdf")), which produces a diagram-only PDF with no embedded BOM table. That matches the PNG/SVG behavior and is the right scope for a fork that already exposes HTML+SVG embed for richer output. Embedding the BOM in PDF would require a full document-composition step (PIL or reportlab) that's well outside the scope of "implement the missing format flag" — and HTML output exists for users who want diagram + BOM in one artifact. No code change; docstring only. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
492dae8 to
10baef3
Compare
Summary
Ports upstream PR #367 — implements the `pdf` output format that's been a TODO stub since v0.4.1. Pipes the graph through Graphviz's PDF renderer and dispatches the bytes the same way PNG goes: file in normal mode, `sys.stdout.buffer` in stdout mode.
Usage
```bash
wireviz -f P harness.yml # produces harness.pdf
cat harness.yml | wireviz -f P -O - - # binary PDF to stdout
```
Changes
Differences from upstream PR wireviz#367
The upstream patch went through the old `graph.render()` + temp-file path — this port uses the in-memory `graph.pipe()` wired up by the stdin/stdout refactor (PR #2 in this fork), so PDF works in both file AND stdout modes without extra plumbing. Also doesn't commit the example PDF artifacts — those are graphviz-version-dependent like SVG/PNG, and the project's "owner will rebuild" convention applies.
Verification
🤖 Generated with Claude Code