From 17ef5806f11d766cc5ce55f0418063e6511cd4d9 Mon Sep 17 00:00:00 2001 From: Ryan Malloy Date: Fri, 13 Feb 2026 03:12:14 -0700 Subject: [PATCH] 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 ---