From 6198f7352a603145940d5ddecb17e21ac6fc63c3 Mon Sep 17 00:00:00 2001 From: Ryan Malloy Date: Fri, 13 Feb 2026 02:09:37 -0700 Subject: [PATCH 1/2] Port pin safety fixes from v0.4.1 to v0.5-dev (issue #432) Add normalize_pin() for consistent int/str coercion at YAML boundary, resolve_pin() for label-to-number resolution, and fix GraphViz port index rendering for loops and shorts on non-sequential connectors. Key changes: - wv_utils: add normalize_pin(), reject floats/bools, refactor expand() - wv_dataclasses: normalize pins/pinlabels/wirelabels in __post_init__, add Connector.resolve_pin(), resolve loop/short pins through labels, detect duplicate pins and minimum pin count in loops/shorts - wv_graphviz: convert pin IDs to position indices in loop/short edges, fix short membership test from pin.index to pin.id - wv_harness: delegate connect() pin resolution to resolve_pin() - tests: 50 tests covering resolution, coercion, rendering, validation --- src/wireviz/wv_dataclasses.py | 125 ++++++--- src/wireviz/wv_graphviz.py | 18 +- src/wireviz/wv_harness.py | 33 +-- src/wireviz/wv_utils.py | 27 +- tests/test_resolve_pin.py | 472 ++++++++++++++++++++++++++++++++++ 5 files changed, 607 insertions(+), 68 deletions(-) create mode 100644 tests/test_resolve_pin.py diff --git a/src/wireviz/wv_dataclasses.py b/src/wireviz/wv_dataclasses.py index 37d53a9..7848e5b 100644 --- a/src/wireviz/wv_dataclasses.py +++ b/src/wireviz/wv_dataclasses.py @@ -25,6 +25,7 @@ from wireviz.wv_utils import ( aspect_ratio, awg_equiv, mm2_equiv, + normalize_pin, parse_number_and_unit, remove_links, ) @@ -409,6 +410,13 @@ class Connector(TopLevelGraphicalComponent): elif self.strip is None: self.strip = Strip() + # Normalize pin-like fields so int/str types are consistent + # regardless of YAML quoting (e.g. "1" vs 1). + if self.pins: + self.pins = [normalize_pin(p) for p in self.pins] + if self.pinlabels: + self.pinlabels = [normalize_pin(p) for p in self.pinlabels] + self.ports_left = False self.ports_right = False self.visible_pins = {} @@ -478,41 +486,43 @@ class Connector(TopLevelGraphicalComponent): loDict[key] = self.loops[loIndex] self.loops = loDict - # TODO: allow using pin labels in addition to pin numbers, - # just like when defining regular connections - # TODO: include properties of wire used to create the loop - for loopName in self.loops: - for pin in self.loops[loopName]: - if pin not in self.pins: - raise Exception( - f'Unknown loop pin "{pin}" for connector "{self.designator}"!' - ) - # Make sure loop connected pins are not hidden. - self.activate_pin(pin, None) - for short in self.shorts: - for pin in self.shorts[short]: - if pin not in self.pins: - raise Exception( - f'Unknown loop pin "{pin}" for connector "{self.designator}"!' - ) - # Make sure loop connected pins are not hidden. - self.activate_pin(pin, None) + # Resolve loop pins: normalize, resolve labels, validate + for loopName, loPins in self.loops.items(): + loPins = [normalize_pin(p) for p in loPins] + resolved = [self.resolve_pin(p) for p in loPins] + if len(resolved) < 2: + raise Exception( + f'Loop "{loopName}" in connector "{self.designator}" ' + f'must connect at least 2 pins (got {len(resolved)}).' + ) + if len(resolved) != len(set(resolved)): + duplicates = [p for p in set(resolved) if resolved.count(p) > 1] + raise Exception( + f'Loop "{loopName}" in connector "{self.designator}" ' + f'contains duplicate pin(s): {duplicates}.' + ) + for pin in resolved: + self.activate_pin(pin, None, is_connection=False) + self.loops[loopName] = resolved - # TODO: Remove the outcommented code here if it is no longer needed as reference - # for loop in self.loops: - # # TODO: allow using pin labels in addition to pin numbers, - # # just like when defining regular connections - # # TODO: include properties of wire used to create the loop - # if len(loop) != 2: - # raise Exception("Loops must be between exactly two pins!") - # for pin in loop: - # if pin not in self.pins: - # raise Exception( - # f'Unknown loop pin "{pin}" for connector "{self.name}"!' - # ) - # # Make sure loop connected pins are not hidden. - # # side=None, determine side to show loops during rendering - # self.activate_pin(pin, side=None, is_connection=True) + # Resolve short pins: normalize, resolve labels, validate + for shortName, shPins in self.shorts.items(): + shPins = [normalize_pin(p) for p in shPins] + resolved = [self.resolve_pin(p) for p in shPins] + if len(resolved) < 2: + raise Exception( + f'Short "{shortName}" in connector "{self.designator}" ' + f'must connect at least 2 pins (got {len(resolved)}).' + ) + if len(resolved) != len(set(resolved)): + duplicates = [p for p in set(resolved) if resolved.count(p) > 1] + raise Exception( + f'Short "{shortName}" in connector "{self.designator}" ' + f'contains duplicate pin(s): {duplicates}.' + ) + for pin in resolved: + self.activate_pin(pin, None, is_connection=False) + self.shorts[shortName] = resolved for i, item in enumerate(self.additional_components): if isinstance(item, dict): @@ -526,6 +536,52 @@ class Connector(TopLevelGraphicalComponent): elif side == Side.RIGHT: self.ports_right = True + def resolve_pin(self, pin: Pin) -> Pin: + """Resolve a pin identifier to its canonical pin number. + + Given a value that may be either a pin number (from self.pins) + or a pin label (from self.pinlabels), returns the corresponding + pin number from self.pins. + + Resolution order: + 1. Value in both pins and pinlabels at the same position -> return directly. + 2. Value in both at different positions -> raise. + 3. Value only in pinlabels -> return corresponding pin number. + 4. Value only in pins -> return directly. + 5. Not found -> raise. + """ + pin = normalize_pin(pin) + in_pins = pin in self.pins + in_labels = pin in self.pinlabels if self.pinlabels else False + + if in_pins and in_labels: + if self.pinlabels.count(pin) > 1: + raise Exception( + f'Pin label "{pin}" in connector "{self.designator}" ' + f"is defined more than once in pinlabels." + ) + if self.pins.index(pin) != self.pinlabels.index(pin): + raise Exception( + f'"{pin}" in connector "{self.designator}" exists in both ' + f"pins and pinlabels at different positions." + ) + return pin + + if in_labels: + if self.pinlabels.count(pin) > 1: + raise Exception( + f'Pin label "{pin}" in connector "{self.designator}" ' + f"is defined more than once." + ) + return self.pins[self.pinlabels.index(pin)] + + if in_pins: + return pin + + raise Exception( + f'Unknown pin "{pin}" for connector "{self.designator}"!' + ) + def compute_qty_multipliers(self): # do not run before all connections in harness have been made! num_populated_pins = len( @@ -777,6 +833,7 @@ class Cable(TopLevelGraphicalComponent): self.wirecount = len(self.colors) if self.wirelabels: + self.wirelabels = [normalize_pin(w) for w in self.wirelabels] if self.shield and "s" in self.wirelabels: raise Exception( '"s" may not be used as a wire label for a shielded cable.' diff --git a/src/wireviz/wv_graphviz.py b/src/wireviz/wv_graphviz.py index aff9ee6..b3da74e 100644 --- a/src/wireviz/wv_graphviz.py +++ b/src/wireviz/wv_graphviz.py @@ -305,7 +305,7 @@ def gv_pin_table(component) -> Table: def gv_short_row_part(pin, connector) -> List: short_row = [] # Td("ADA"), Td("DAD") for short, shPins in connector.shorts.items(): - if pin.index + 1 in shPins: + if pin.id in shPins: short_row.append(Td("", port=f"p{pin.index+1}j")) else: short_row.append(Td("")) @@ -344,8 +344,13 @@ def gv_connector_loops(connector: Connector) -> List: loColor = comp.color.html for i in range(1, len(loPins)): - head = f"{connector.designator}:p{loPins[i - 1]}{loop_side}:{loop_dir}" - tail = f"{connector.designator}:p{loPins[i]}{loop_side}:{loop_dir}" + # Convert pin IDs to 1-based port indices. + # Ports are named p{index+1} in the connector table, + # not p{pin_id} — these differ for non-sequential pins. + idx_prev = connector.pin_objects[loPins[i - 1]].index + 1 + idx_curr = connector.pin_objects[loPins[i]].index + 1 + head = f"{connector.designator}:p{idx_prev}{loop_side}:{loop_dir}" + tail = f"{connector.designator}:p{idx_curr}{loop_side}:{loop_dir}" loop_edges.append((head, tail, loColor)) return loop_edges @@ -360,8 +365,11 @@ def gv_connector_shorts(connector: Connector) -> List: shColor = f"#FFFFFF:{comp.color.html}:#FFFFFF" for i in range(1, len(shPins)): - head = f"{connector.designator}:p{shPins[i - 1]}j:c" - tail = f"{connector.designator}:p{shPins[i]}j:c" + # Convert pin IDs to 1-based port indices (same fix as loops). + idx_prev = connector.pin_objects[shPins[i - 1]].index + 1 + idx_curr = connector.pin_objects[shPins[i]].index + 1 + head = f"{connector.designator}:p{idx_prev}j:c" + tail = f"{connector.designator}:p{idx_curr}j:c" short_edges.append((head, tail, shColor)) return short_edges diff --git a/src/wireviz/wv_harness.py b/src/wireviz/wv_harness.py index 363c4d8..6c51e89 100644 --- a/src/wireviz/wv_harness.py +++ b/src/wireviz/wv_harness.py @@ -236,30 +236,17 @@ class Harness: to_name: str, to_pin: Union[int, str], ) -> None: - # check from and to connectors - for name, pin in zip([from_name, to_name], [from_pin, to_pin]): + # resolve pin labels to pin numbers via Connector.resolve_pin() + for name, pin, is_from in [ + (from_name, from_pin, True), + (to_name, to_pin, False), + ]: if name is not None and name in self.connectors: - connector = self.connectors[name] - # check if provided name is ambiguous - if pin in connector.pins and pin in connector.pinlabels: - if connector.pins.index(pin) != connector.pinlabels.index(pin): - raise Exception( - f"{name}:{pin} is defined both in pinlabels and pins, " - "for different pins." - ) - # TODO: Maybe issue a warning if present in both lists - # but referencing the same pin? - if pin in connector.pinlabels: - if connector.pinlabels.count(pin) > 1: - raise Exception(f"{name}:{pin} is defined more than once.") - index = connector.pinlabels.index(pin) - pin = connector.pins[index] # map pin name to pin number - if name == from_name: - from_pin = pin - if name == to_name: - to_pin = pin - if not pin in connector.pins: - raise Exception(f"{name}:{pin} not found.") + resolved = self.connectors[name].resolve_pin(pin) + if is_from: + from_pin = resolved + else: + to_pin = resolved # check via cable if via_name in self.cables: diff --git a/src/wireviz/wv_utils.py b/src/wireviz/wv_utils.py index 3ac71ec..3e40b02 100644 --- a/src/wireviz/wv_utils.py +++ b/src/wireviz/wv_utils.py @@ -37,6 +37,25 @@ def mm2_equiv(awg): return mm2_equiv_table.get(str(awg), "Unknown") +def normalize_pin(value): + """Normalize a pin value: try int() first, fall back to str(). + + Rejects bools and non-integer floats to prevent silent data loss. + Matches the coercion convention used by expand(). + """ + if isinstance(value, bool): + raise ValueError(f"Boolean {value!r} is not a valid pin identifier") + if isinstance(value, float) and not value.is_integer(): + raise ValueError( + f"Float {value!r} is not a valid pin identifier " + f"(would be silently truncated to {int(value)})" + ) + try: + return int(value) + except (ValueError, TypeError): + return str(value) if value is not None else value + + def expand(yaml_data): # yaml_data can be: # - a singleton (normally str or int) @@ -60,15 +79,11 @@ def expand(yaml_data): output.append(x) # descending range else: # a == b output.append(a) # range of length 1 - except: + except (ValueError, TypeError): # '-' was not a delimiter between two ints, pass e through unchanged output.append(e) else: - try: - x = int(e) # single int - except Exception: - x = e # string - output.append(x) + output.append(normalize_pin(e)) return output diff --git a/tests/test_resolve_pin.py b/tests/test_resolve_pin.py new file mode 100644 index 0000000..48c6524 --- /dev/null +++ b/tests/test_resolve_pin.py @@ -0,0 +1,472 @@ +# -*- coding: utf-8 -*- + +"""Tests for Connector.resolve_pin() and loop/short pin resolution. + +Port of the v0.4.1 test suite (issue #432) adapted for v0.5-dev architecture: +- designator= instead of name= +- loops/shorts are dicts (auto-keyed from lists) +- pin_objects dict with PinClass instances +- _num_connections instead of visible_pins +- activate_pin() has is_connection parameter +""" + +import pytest + +from wireviz.wv_dataclasses import Cable, Connector + + +def make_connector(pins=None, pinlabels=None, loops=None, shorts=None, **kwargs): + """Helper to build a Connector with minimal required fields.""" + args = {"designator": "X1"} + if pins is not None: + args["pins"] = pins + if pinlabels is not None: + args["pinlabels"] = pinlabels + if loops is not None: + args["loops"] = loops + if shorts is not None: + args["shorts"] = shorts + args.update(kwargs) + return Connector(**args) + + +# --- resolve_pin() happy paths --- + + +class TestResolvePinHappyPaths: + def test_pin_number_passthrough(self): + c = make_connector(pins=[1, 2, 3]) + assert c.resolve_pin(2) == 2 + + def test_label_resolves_to_pin_number(self): + c = make_connector(pins=[1, 2, 3], pinlabels=["VCC", "GND", "SIG"]) + assert c.resolve_pin("GND") == 2 + + def test_label_with_non_sequential_pins(self): + c = make_connector(pins=[10, 20, 30], pinlabels=["A", "B", "C"]) + assert c.resolve_pin("B") == 20 + + def test_value_in_both_lists_same_position(self): + c = make_connector(pins=["A", "B", "C"], pinlabels=["A", "X", "Y"]) + assert c.resolve_pin("A") == "A" + + def test_empty_pinlabels_falls_through(self): + c = make_connector(pins=[1, 2, 3], pinlabels=[]) + assert c.resolve_pin(3) == 3 + + +# --- resolve_pin() error paths --- + + +class TestResolvePinErrors: + def test_unknown_pin_raises(self): + c = make_connector(pins=[1, 2, 3], pinlabels=["A", "B", "C"]) + with pytest.raises(Exception, match="Unknown pin"): + c.resolve_pin("NONEXISTENT") + + def test_unknown_number_raises(self): + c = make_connector(pins=[1, 2, 3]) + with pytest.raises(Exception, match="Unknown pin"): + c.resolve_pin(99) + + def test_ambiguous_pin_different_positions(self): + c = make_connector(pins=["A", "B", "C"], pinlabels=["X", "A", "Y"]) + with pytest.raises(Exception, match="exists in both"): + c.resolve_pin("A") + + def test_duplicate_label_only_in_labels(self): + c = make_connector(pins=[1, 2, 3], pinlabels=["A", "B", "A"]) + with pytest.raises(Exception, match="defined more than once"): + c.resolve_pin("A") + + def test_duplicate_label_in_both_lists(self): + c = make_connector(pins=["A", "B", "C"], pinlabels=["A", "X", "A"]) + with pytest.raises(Exception, match="defined more than once"): + c.resolve_pin("A") + + +# --- Loop resolution (dict format in v0.5) --- + + +class TestLoopResolution: + def test_loop_with_labels(self): + c = make_connector( + pins=[1, 2, 3, 4], + pinlabels=["VCC", "GND", "TX", "RX"], + loops=[["VCC", "GND"]], + ) + # v0.5 auto-converts list to dict with AutoLO keys + assert list(c.loops.values())[0] == [1, 2] + + def test_loop_with_mixed_number_and_label(self): + c = make_connector( + pins=[1, 2, 3, 4], + pinlabels=["VCC", "GND", "TX", "RX"], + loops=[[1, "GND"]], + ) + assert list(c.loops.values())[0] == [1, 2] + + def test_loop_with_non_sequential_pins(self): + c = make_connector( + pins=[10, 20, 30, 40], + pinlabels=["VCC", "GND", "TX", "RX"], + loops=[["VCC", "GND"]], + ) + assert list(c.loops.values())[0] == [10, 20] + + def test_loop_self_reference_raises(self): + with pytest.raises(Exception, match="duplicate pin"): + make_connector( + pins=[1, 2, 3], + pinlabels=["A", "B", "C"], + loops=[["A", "A"]], + ) + + def test_loop_self_reference_via_label_and_number(self): + with pytest.raises(Exception, match="duplicate pin"): + make_connector( + pins=[1, 2, 3], + pinlabels=["A", "B", "C"], + loops=[[2, "B"]], + ) + + def test_loop_activates_pins(self): + c = make_connector( + pins=[1, 2, 3, 4], + pinlabels=["VCC", "GND", "TX", "RX"], + loops=[["TX", "RX"]], + ) + # In v0.5, activation is tracked via _num_connections on pin_objects. + # Loops use is_connection=False, so _num_connections stays 0, + # but the pin should still be accessible (not hidden). + # The key check: pins are found in pin_objects (always true), + # and the loop was processed without error. + assert 3 in c.pin_objects + assert 4 in c.pin_objects + + def test_multiple_loops(self): + c = make_connector( + pins=[1, 2, 3, 4, 5, 6], + pinlabels=["A", "B", "C", "D", "E", "F"], + loops=[["A", "B"], ["E", "F"]], + ) + values = list(c.loops.values()) + assert values[0] == [1, 2] + assert values[1] == [5, 6] + + def test_loop_dict_format(self): + """Loops can be specified as dicts directly (named loops).""" + c = make_connector( + pins=[1, 2, 3, 4], + pinlabels=["VCC", "GND", "TX", "RX"], + loops={"MyLoop": ["VCC", "GND"]}, + ) + assert c.loops["MyLoop"] == [1, 2] + + def test_multi_pin_loop(self): + """v0.5 allows >2 pins per loop.""" + c = make_connector( + pins=[1, 2, 3, 4], + pinlabels=["A", "B", "C", "D"], + loops=[["A", "B", "C"]], + ) + assert list(c.loops.values())[0] == [1, 2, 3] + + +# --- Short resolution (new in v0.5) --- + + +class TestShortResolution: + def test_short_with_labels(self): + c = make_connector( + pins=[1, 2, 3, 4], + pinlabels=["VCC", "GND", "TX", "RX"], + shorts=[["VCC", "GND"]], + ) + assert list(c.shorts.values())[0] == [1, 2] + + def test_short_with_non_sequential_pins(self): + c = make_connector( + pins=[10, 20, 30, 40], + pinlabels=["A", "B", "C", "D"], + shorts=[["A", "C"]], + ) + assert list(c.shorts.values())[0] == [10, 30] + + def test_short_self_reference_raises(self): + with pytest.raises(Exception, match="duplicate pin"): + make_connector( + pins=[1, 2, 3], + pinlabels=["A", "B", "C"], + shorts=[["A", "A"]], + ) + + def test_short_dict_format(self): + c = make_connector( + pins=[1, 2, 3], + pinlabels=["A", "B", "C"], + shorts={"MyShort": ["A", "C"]}, + ) + assert c.shorts["MyShort"] == [1, 3] + + def test_multiple_shorts(self): + c = make_connector( + pins=[1, 2, 3, 4], + pinlabels=["A", "B", "C", "D"], + shorts=[["A", "B"], ["C", "D"]], + ) + values = list(c.shorts.values()) + assert values[0] == [1, 2] + assert values[1] == [3, 4] + + +# --- Pin type coercion --- + + +class TestPinTypeCoercion: + def test_str_numeric_pins_normalize_to_int(self): + c = make_connector(pins=["1", "2", "3"]) + assert c.pins == [1, 2, 3] + assert all(isinstance(p, int) for p in c.pins) + + def test_mixed_type_pins_normalize(self): + c = make_connector(pins=[1, "2", 3]) + assert c.pins == [1, 2, 3] + + def test_leading_zeros_normalize(self): + c = make_connector(pins=["01", "02", "03"]) + assert c.pins == [1, 2, 3] + + def test_non_numeric_pins_stay_str(self): + c = make_connector(pins=["A", "B", "C"]) + assert c.pins == ["A", "B", "C"] + assert all(isinstance(p, str) for p in c.pins) + + def test_duplicate_after_normalization_raises(self): + with pytest.raises(Exception, match="Pins are not unique"): + make_connector(pins=[1, "1"]) + + def test_pinlabels_normalize(self): + c = make_connector(pins=[1, 2], pinlabels=["10", "20"]) + assert c.pinlabels == [10, 20] + + def test_loop_pins_normalize(self): + c = make_connector( + pins=[1, 2, 3, 4], + loops=[["1", "2"]], + ) + assert list(c.loops.values())[0] == [1, 2] + + def test_str_loop_pins_match_auto_generated_int_pins(self): + """String loop pins match auto-generated sequential int pins.""" + c = make_connector( + pincount=4, + loops=[["1", "3"]], + ) + assert list(c.loops.values())[0] == [1, 3] + + def test_short_pins_normalize(self): + c = make_connector( + pins=[1, 2, 3, 4], + shorts=[["1", "2"]], + ) + assert list(c.shorts.values())[0] == [1, 2] + + def test_wirelabels_normalize(self): + cable = Cable( + designator="W1", wirecount=3, + colors=["BK", "RD", "GN"], + wirelabels=["1", "2", "3"], + ) + assert cable.wirelabels == [1, 2, 3] + + +# --- Loop/Short rendering: non-sequential pins --- + + +class TestRendering: + """Verify GraphViz output uses port indices, not pin numbers.""" + + def _make_harness(self): + from wireviz.wv_dataclasses import Metadata, Options, Tweak + from wireviz.wv_harness import Harness + return Harness( + metadata=Metadata({}), + options=Options(), + tweak=Tweak(), + ) + + def test_loop_non_sequential_pins_use_indices(self): + import re + harness = self._make_harness() + harness.add_connector( + "X1", + pins=[10, 20, 30, 40], + pinlabels=["VCC", "GND", "TX", "RX"], + loops=[["VCC", "GND"]], + ) + harness.add_cable("W1", wirecount=2, colors=["BK", "RD"]) + harness.connect("X1", "TX", "W1", 1, None, None) + harness.connect(None, None, "W1", 2, "X1", "RX") + graph = harness.create_graph() + gv = graph.source + + # Loop should reference p1 and p2 (indices), not p10 and p20 + loop_edges = re.findall(r"X1:p(\d+)\w:\w -- X1:p(\d+)\w:\w", gv) + assert len(loop_edges) >= 1, f"Expected loop edge, found none in:\n{gv}" + idx_a, idx_b = loop_edges[0] + assert idx_a == "1" and idx_b == "2", ( + f"Loop ports should be p1/p2 (indices), got p{idx_a}/p{idx_b}" + ) + + def test_short_non_sequential_pins_use_indices(self): + import re + harness = self._make_harness() + harness.add_connector( + "X1", + pins=[10, 20, 30, 40], + pinlabels=["VCC", "GND", "TX", "RX"], + shorts=[["VCC", "GND"]], + ) + harness.add_cable("W1", wirecount=2, colors=["BK", "RD"]) + harness.connect("X1", "TX", "W1", 1, None, None) + harness.connect(None, None, "W1", 2, "X1", "RX") + graph = harness.create_graph() + gv = graph.source + + # Short should reference p1j and p2j (indices), not p10j and p20j + short_edges = re.findall(r"X1:p(\d+)j:c -- X1:p(\d+)j:c", gv) + assert len(short_edges) >= 1, f"Expected short edge, found none in:\n{gv}" + idx_a, idx_b = short_edges[0] + assert idx_a == "1" and idx_b == "2", ( + f"Short ports should be p1j/p2j (indices), got p{idx_a}j/p{idx_b}j" + ) + + def test_sequential_pins_still_work(self): + harness = self._make_harness() + harness.add_connector( + "X1", + pinlabels=["VCC", "GND", "TX", "RX"], + loops=[["VCC", "GND"]], + ) + harness.add_cable("W1", wirecount=2, colors=["BK", "RD"]) + harness.connect("X1", "TX", "W1", 1, None, None) + harness.connect(None, None, "W1", 2, "X1", "RX") + graph = harness.create_graph() + gv = graph.source + assert ":p1" in gv + assert ":p2" in gv + + +# --- Harness.connect() delegation --- + + +class TestHarnessConnectDelegation: + def _make_harness(self): + from wireviz.wv_dataclasses import Metadata, Options, Tweak + from wireviz.wv_harness import Harness + return Harness( + metadata=Metadata({}), + options=Options(), + tweak=Tweak(), + ) + + def test_connect_resolves_labels(self): + harness = self._make_harness() + harness.add_connector( + "X1", pins=[1, 2, 3], pinlabels=["VCC", "GND", "SIG"] + ) + harness.add_connector("X2", pins=[1, 2, 3]) + harness.add_cable("W1", wirecount=1, colors=["BK"]) + + harness.connect("X1", "SIG", "W1", 1, "X2", 1) + + # The connection should store a PinClass with the resolved pin id (3) + conn = harness.cables["W1"]._connections[0] + assert conn.from_.id == 3 + + def test_connect_rejects_unknown_pin(self): + harness = self._make_harness() + harness.add_connector("X1", pins=[1, 2, 3]) + harness.add_connector("X2", pins=[1, 2, 3]) + harness.add_cable("W1", wirecount=1, colors=["BK"]) + + with pytest.raises(Exception, match="Unknown pin"): + harness.connect("X1", 99, "W1", 1, "X2", 1) + + def test_connect_normalizes_string_pin(self): + """Programmatic callers passing str '3' should match normalized int 3.""" + harness = self._make_harness() + harness.add_connector("X1", pins=[1, 2, 3]) + harness.add_connector("X2", pins=[1, 2, 3]) + harness.add_cable("W1", wirecount=1, colors=["BK"]) + + harness.connect("X1", "3", "W1", 1, "X2", 1) + conn = harness.cables["W1"]._connections[0] + assert conn.from_.id == 3 + + +# --- Apollo review: validation edge cases --- + + +class TestApolloValidation: + """Tests added per Apollo code review findings.""" + + def test_partial_duplicate_loop_raises(self): + """C1: Loop [A, B, A] -> [1, 2, 1] has duplicate pin 1.""" + with pytest.raises(Exception, match="duplicate pin"): + make_connector( + pins=[1, 2, 3], + pinlabels=["A", "B", "C"], + loops=[["A", "B", "A"]], + ) + + def test_partial_duplicate_short_raises(self): + """C1: Short with partial duplicates detected.""" + with pytest.raises(Exception, match="duplicate pin"): + make_connector( + pins=[1, 2, 3], + pinlabels=["A", "B", "C"], + shorts=[["A", "B", "A"]], + ) + + def test_empty_loop_raises(self): + """C2: Empty loop has no physical meaning.""" + with pytest.raises(Exception, match="at least 2 pins"): + make_connector(pins=[1, 2, 3], loops=[[]]) + + def test_single_pin_loop_raises(self): + """C2: Single-pin loop has no physical meaning.""" + with pytest.raises(Exception, match="at least 2 pins"): + make_connector(pins=[1, 2, 3], loops=[[1]]) + + def test_empty_short_raises(self): + """C2: Empty short has no physical meaning.""" + with pytest.raises(Exception, match="at least 2 pins"): + make_connector(pins=[1, 2, 3], shorts=[[]]) + + def test_single_pin_short_raises(self): + """C2: Single-pin short has no physical meaning.""" + with pytest.raises(Exception, match="at least 2 pins"): + make_connector(pins=[1, 2, 3], shorts=[[1]]) + + def test_float_pin_raises(self): + """C3: Non-integer float pin should be rejected, not truncated.""" + with pytest.raises(ValueError, match="Float"): + make_connector(pins=[3.5, 2, 3]) + + def test_bool_pin_raises(self): + """C3: Boolean pin should be rejected, not coerced to int.""" + with pytest.raises(ValueError, match="Boolean"): + make_connector(pins=[True, 2, 3]) + + def test_integer_float_pin_accepted(self): + """C3: Integer-valued float (e.g. 3.0) should normalize to int 3.""" + c = make_connector(pins=[1.0, 2.0, 3.0]) + assert c.pins == [1, 2, 3] + assert all(isinstance(p, int) for p in c.pins) + + def test_resolve_pin_normalizes_input(self): + """I1: resolve_pin() normalizes at its own boundary.""" + c = make_connector(pins=[1, 2, 3]) + assert c.resolve_pin("2") == 2 From 17ef5806f11d766cc5ce55f0418063e6511cd4d9 Mon Sep 17 00:00:00 2001 From: Ryan Malloy Date: Fri, 13 Feb 2026 03:12:14 -0700 Subject: [PATCH 2/2] Address Apollo review deferred items (S2, I4, S5) S2: Replace type() == list with isinstance() for list-to-dict conversion. I4: Fix duplicate GraphViz port names when a pin appears in multiple shorts. Port names now encode short column index (p{idx}j{col}) so each cell in the shorts grid has a unique port. Edge generation in gv_connector_shorts() updated to match. S5: Add rendering tests for multi-pin loop edge chains and multi-short port name uniqueness. --- src/wireviz/wv_dataclasses.py | 4 +-- src/wireviz/wv_graphviz.py | 15 +++++---- tests/test_resolve_pin.py | 60 +++++++++++++++++++++++++++++++++-- 3 files changed, 68 insertions(+), 11 deletions(-) diff --git a/src/wireviz/wv_dataclasses.py b/src/wireviz/wv_dataclasses.py index 7848e5b..7566b82 100644 --- a/src/wireviz/wv_dataclasses.py +++ b/src/wireviz/wv_dataclasses.py @@ -470,7 +470,7 @@ class Connector(TopLevelGraphicalComponent): self.show_pincount = self.style != "simple" # Convert short List to Short Dict - if type(self.shorts) == list: + if isinstance(self.shorts, list): self.shorts_hide_lable = True shDict = dict() for shIndex in range(0, len(self.shorts)): @@ -479,7 +479,7 @@ class Connector(TopLevelGraphicalComponent): self.shorts = shDict # Convert loop List to loop Dict - if type(self.loops) == list: + if isinstance(self.loops, list): loDict = dict() for loIndex in range(0, len(self.loops)): key = "AutoLO" + str(loIndex) diff --git a/src/wireviz/wv_graphviz.py b/src/wireviz/wv_graphviz.py index b3da74e..cea7829 100644 --- a/src/wireviz/wv_graphviz.py +++ b/src/wireviz/wv_graphviz.py @@ -303,10 +303,12 @@ def gv_pin_table(component) -> Table: def gv_short_row_part(pin, connector) -> List: - short_row = [] # Td("ADA"), Td("DAD") - for short, shPins in connector.shorts.items(): + short_row = [] + for sh_col, (short, shPins) in enumerate(connector.shorts.items()): if pin.id in shPins: - short_row.append(Td("", port=f"p{pin.index+1}j")) + # Port name encodes pin index + short column to stay unique + # when the same pin appears in multiple shorts. + short_row.append(Td("", port=f"p{pin.index+1}j{sh_col}")) else: short_row.append(Td("")) return short_row if len(short_row) > 0 else None @@ -358,7 +360,7 @@ def gv_connector_loops(connector: Connector) -> List: def gv_connector_shorts(connector: Connector) -> List: short_edges = [] - for short, shPins in connector.shorts.items(): + for sh_col, (short, shPins) in enumerate(connector.shorts.items()): comp = getAddCompFromRef(short, connector) shColor = "#FFFFFF:#000000:#FFFFFF" if comp != None and comp.color != None: @@ -366,10 +368,11 @@ def gv_connector_shorts(connector: Connector) -> List: for i in range(1, len(shPins)): # Convert pin IDs to 1-based port indices (same fix as loops). + # Port name includes short column index for uniqueness. idx_prev = connector.pin_objects[shPins[i - 1]].index + 1 idx_curr = connector.pin_objects[shPins[i]].index + 1 - head = f"{connector.designator}:p{idx_prev}j:c" - tail = f"{connector.designator}:p{idx_curr}j:c" + head = f"{connector.designator}:p{idx_prev}j{sh_col}:c" + tail = f"{connector.designator}:p{idx_curr}j{sh_col}:c" short_edges.append((head, tail, shColor)) return short_edges diff --git a/tests/test_resolve_pin.py b/tests/test_resolve_pin.py index 48c6524..1acd7c8 100644 --- a/tests/test_resolve_pin.py +++ b/tests/test_resolve_pin.py @@ -334,12 +334,13 @@ class TestRendering: graph = harness.create_graph() gv = graph.source - # Short should reference p1j and p2j (indices), not p10j and p20j - short_edges = re.findall(r"X1:p(\d+)j:c -- X1:p(\d+)j:c", gv) + # Short should reference p1j0 and p2j0 (index + column 0), + # not p10j0 and p20j0 (pin IDs) + short_edges = re.findall(r"X1:p(\d+)j\d+:c -- X1:p(\d+)j\d+:c", gv) assert len(short_edges) >= 1, f"Expected short edge, found none in:\n{gv}" idx_a, idx_b = short_edges[0] assert idx_a == "1" and idx_b == "2", ( - f"Short ports should be p1j/p2j (indices), got p{idx_a}j/p{idx_b}j" + f"Short ports should be p1/p2 (indices), got p{idx_a}/p{idx_b}" ) def test_sequential_pins_still_work(self): @@ -357,6 +358,59 @@ class TestRendering: assert ":p1" in gv assert ":p2" in gv + def test_multi_pin_loop_renders_chain(self): + """S5: >2 pin loop produces correct edge chain (p1->p2, p2->p3).""" + import re + harness = self._make_harness() + harness.add_connector( + "X1", + pins=[10, 20, 30, 40], + pinlabels=["A", "B", "C", "D"], + loops=[["A", "B", "C"]], + ) + harness.add_cable("W1", wirecount=1, colors=["BK"]) + harness.connect("X1", "D", "W1", 1, None, None) + graph = harness.create_graph() + gv = graph.source + + loop_edges = re.findall(r"X1:p(\d+)\w:\w -- X1:p(\d+)\w:\w", gv) + # 3-pin loop should produce 2 edges: p1->p2 and p2->p3 + assert len(loop_edges) == 2, ( + f"Expected 2 loop edges for 3-pin loop, got {len(loop_edges)}" + ) + assert loop_edges[0] == ("1", "2") + assert loop_edges[1] == ("2", "3") + + def test_pin_in_multiple_shorts_unique_ports(self): + """I4: pin in two shorts gets unique port names per column.""" + import re + harness = self._make_harness() + harness.add_connector( + "X1", + pins=[1, 2, 3, 4], + shorts={"SH1": [1, 2], "SH2": [1, 3]}, + ) + harness.add_cable("W1", wirecount=1, colors=["BK"]) + harness.connect("X1", 4, "W1", 1, None, None) + graph = harness.create_graph() + gv = graph.source + + # SH1 edges should use column 0 (j0), SH2 should use column 1 (j1) + sh1_edges = re.findall(r"X1:p(\d+)j0:c -- X1:p(\d+)j0:c", gv) + sh2_edges = re.findall(r"X1:p(\d+)j1:c -- X1:p(\d+)j1:c", gv) + assert len(sh1_edges) >= 1, f"Expected SH1 edge (j0), found none in:\n{gv}" + assert len(sh2_edges) >= 1, f"Expected SH2 edge (j1), found none in:\n{gv}" + # SH1: pin 1 (idx 1) -> pin 2 (idx 2) + assert sh1_edges[0] == ("1", "2") + # SH2: pin 1 (idx 1) -> pin 3 (idx 3) + assert sh2_edges[0] == ("1", "3") + + # Verify no duplicate port names in the HTML table + port_names = re.findall(r'PORT="(p\d+j\d+)"', gv) + assert len(port_names) == len(set(port_names)), ( + f"Duplicate port names found: {port_names}" + ) + # --- Harness.connect() delegation ---