Skip to content

Wire up PDF output (port of upstream PR #367)#8

Merged
SomethingNew71 merged 2 commits into
port-321-stdin-stdoutfrom
port-367-pdf-output
May 5, 2026
Merged

Wire up PDF output (port of upstream PR #367)#8
SomethingNew71 merged 2 commits into
port-321-stdin-stdoutfrom
port-367-pdf-output

Conversation

@SomethingNew71
Copy link
Copy Markdown

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

  • `format_codes`: un-commented `"P": "pdf"` (use `-f P` on the CLI)
  • `Harness._render()`: new `pdf` branch using `graph.pipe(format="pdf")`
  • `Harness.output()`: removed the "PDF output is not yet supported" stderr warning

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

  • ✅ `wireviz -f P harness.yml` produces a valid PDF (file v1.7)
  • ✅ `cat harness.yml | wireviz -f P -O - -` writes valid PDF to stdout
  • ✅ `build_examples.py` deterministic outputs byte-identical

🤖 Generated with Claude Code

@SomethingNew71
Copy link
Copy Markdown
Author

@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 output_dpi) — appreciate the work.

The port uses the in-memory graph.pipe(format="pdf") path from PR #2 in this fork (the stdin/stdout streaming port of upstream wireviz#321) instead of the graph.render() + temp-file path your original PR used. Side benefit: PDF works for wireviz -f P -O - - (binary PDF to stdout) without any extra plumbing.

Commit attribution links back to your PR. Thanks!

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/wireviz/Harness.py
Comment on lines +763 to +764
if "pdf" in fmt:
outputs["pdf"] = graph.pipe(format="pdf")
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

SomethingNew71 and others added 2 commits May 4, 2026 20:56
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>
@SomethingNew71 SomethingNew71 force-pushed the port-367-pdf-output branch from 492dae8 to 10baef3 Compare May 5, 2026 00:59
@SomethingNew71 SomethingNew71 merged commit c66441e into port-321-stdin-stdout May 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant