Per-connector / per-cable tweak with name placeholder (port of upstream PR #357)#7
Conversation
|
@kvid — heads up, your upstream #357 (per-connector/per-cable tweak with name placeholder) 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. Upstream Adapted faithfully — only renamed the public method Commit attribution links back to your PR. Thanks for the work — clean design. |
There was a problem hiding this comment.
Code Review
This pull request introduces per-connector and per-cable tweak overrides with placeholder substitution, allowing users to define templates that are automatically customized with the specific node's name. The changes include documentation updates, new fields in the Tweak, Connector, and Cable data classes, and the implementation of the _extend_tweak method in Harness.py to handle the merging and substitution logic. Review feedback identified two potential crash scenarios in the new logic: an AttributeError when the substitution lambda processes non-string values (such as null in YAML), and a potential TypeError in graph generation caused by setting override entries to None instead of maintaining them as dictionaries.
| # placeholder; only None falls back. | ||
| if ph is None: | ||
| ph = self.tweak.placeholder | ||
| rph = (lambda s: s.replace(ph, node.name)) if ph else (lambda s: s) |
There was a problem hiding this comment.
The rph lambda will raise an AttributeError if it is called with a None value. This can happen in the override dictionary when an attribute is being deleted (set to null in YAML). The lambda should be updated to handle non-string values gracefully.
| rph = (lambda s: s.replace(ph, node.name)) if ph else (lambda s: s) | |
| rph = (lambda s: s.replace(ph, node.name) if isinstance(s, str) else s) if ph else (lambda s: s) |
| f"{node.name}.tweak.override.{ident}.{k} conflicts with another" | ||
| ) | ||
| s_dict[k] = v | ||
| s_override[ident] = s_dict or None |
There was a problem hiding this comment.
Setting s_override[ident] to None when s_dict is empty will cause a crash in Harness.create_graph() (specifically at line 629) because it expects the values in the override dictionary to be dictionaries, not None. It is safer to keep it as an empty dictionary.
| s_override[ident] = s_dict or None | |
| s_override[ident] = s_dict |
…stream PR wireviz#357) Connectors and cables can now carry their own ``tweak:`` block with the same ``override`` / ``append`` shape as the global one. The harness folds per-node tweaks into the global tweak at instantiation, with an optional placeholder substring rewritten to the node's actual designator — making it practical to author a single tweak template and apply it to many components. Example: tweak: placeholder: "@@" connectors: X1: pinlabels: [A, B] tweak: append: - "@@_extra [color=red, style=dashed];" renders X1's per-node tweak as ``X1_extra [color=red, style=dashed];`` in the .gv source. The same connector definition reused for X2 would produce ``X2_extra ...``. Placeholder semantics: * Per-node ``tweak.placeholder`` overrides the global ``tweak.placeholder``. * An empty string at the per-node level explicitly opts out of substitution for that node. * ``None`` (the default) falls back to the global placeholder. * When neither is set, no substitution happens — bare strings are appended/overridden as-written. Implementation: * DataClasses.py — ``Tweak`` gains ``placeholder: Optional[str] = None``. ``Connector`` and ``Cable`` gain ``tweak: Optional[Tweak] = None``, with ``__post_init__`` coercing a dict literal into a Tweak instance. * Harness.py — new ``Harness._extend_tweak(node)`` method, called from ``add_connector()`` and ``add_cable()``, performs the placeholder substitution and merges into ``self.tweak``. Raises ``ValueError`` if two nodes contribute conflicting overrides for the same key. * docs/syntax.md — documents per-connector / per-cable tweak fields and the new placeholder semantics. Adapted from wireviz#357 (originally by @kvid, targeting upstream ``dev``). Renamed ``extend_tweak`` to ``_extend_tweak`` to mark it private; otherwise faithful to the original logic. ``make_list`` is already in master's wv_bom so no helper backport needed. Verified: * Smoke test with placeholder ``@@`` and per-connector + per-cable ``append:`` blocks produces ``X1_extra``, ``W1_label``, ``cable W1`` in the rendered .gv (the @@'s are substituted). * build_examples.py: deterministic outputs (.gv, .bom.tsv) byte- identical to baseline (no existing example uses the new syntax, so no new substitutions fire). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ack) Addresses gemini-code-assist feedback on #7: * The ``rph`` lambda would raise ``AttributeError`` when called with a None value, which is a legitimate case in YAML when an override deletes a key (``key: null``). Now passes non-strings through unchanged so substitution is a no-op for None / numeric / bool values. * ``s_override[ident] = s_dict or None`` would collapse an empty per-ident override dict to None, which Harness.create_graph() doesn't expect (it iterates ``override.items()`` expecting dict-shaped values). Always store the dict, even when empty — ``self.tweak.override = s_override or None`` already handles the outer "no overrides at all" case. Verified: per-connector override with ``key: null`` now renders without the prior AttributeError. build_examples.py deterministic outputs still byte-identical. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2d5917c to
39129a4
Compare
Summary
Ports upstream PR #357 — connectors and cables can now carry their own `tweak:` block (with the same `override` / `append` shape as the global one), and a placeholder substring gets rewritten to the node's actual designator at instantiation. Makes it practical to author one tweak template and apply it to many components.
Example
```yaml
tweak:
placeholder: "@@"
connectors:
X1:
pinlabels: [A, B]
tweak:
append:
- "@@_extra [color=red, style=dashed];"
```
renders as `X1_extra [color=red, style=dashed];` in the .gv. Same connector definition reused as X2 → `X2_extra ...`.
Placeholder semantics
Implementation
Differences from upstream PR wireviz#357
Renamed `extend_tweak` to `_extend_tweak` to mark it private. Otherwise faithful to the original logic. `make_list` already exists in master's `wv_bom` so no helper backport needed.
Verification
🤖 Generated with Claude Code