Merge pull request #67 from Mehanik/fix/delete-schematic-component-multiline

fix: delete_schematic_component fails on multi-line KiCAD format

python/kicad_interface.py

@@ -755,59 +755,55 @@ class KiCADInterface:
                 }
 
             with open(sch_file, "r", encoding="utf-8") as f:
-                lines = f.read().split("\n")
+                content = f.read()
 
-            # Find lib_symbols range to skip it
+            def find_matching_paren(s, start):
-            lib_sym_start, lib_sym_end = None, None
+                """Find the closing paren matching the opening paren at start."""
-            depth = 0
+                depth = 0
-            for i, line in enumerate(lines):
+                i = start
-                if "(lib_symbols" in line and lib_sym_start is None:
+                while i < len(s):
-                    lib_sym_start = i
+                    if s[i] == "(":
-                    depth = sum(1 for c in line if c == "(") - sum(
+                        depth += 1
-                        1 for c in line if c == ")"
+                    elif s[i] == ")":
-                    )
+                        depth -= 1
-                elif lib_sym_start is not None and lib_sym_end is None:
+                        if depth == 0:
-                    depth += sum(1 for c in line if c == "(") - sum(
+                            return i
-                        1 for c in line if c == ")"
+                    i += 1
-                    )
+                return -1
-                    if depth == 0:
-                        lib_sym_end = i
-                        break
 
-            # Find ALL placed symbol blocks matching the reference (handles duplicates)
+            # Skip lib_symbols section
-            blocks_to_delete = []
+            lib_sym_pos = content.find("(lib_symbols")
-            i = 0
+            lib_sym_end = (
-            while i < len(lines):
+                find_matching_paren(content, lib_sym_pos) if lib_sym_pos >= 0 else -1
-                # Skip lib_symbols
+            )
-                if lib_sym_start is not None and lib_sym_end is not None:
-                    if lib_sym_start <= i <= lib_sym_end:
-                        i += 1
-                        continue
 
-                if re.match(r"\s*\(symbol\s+\(lib_id\s+\"", lines[i]):
+            # Find ALL placed symbol blocks matching the reference (handles duplicates).
-                    b_start = i
+            # Use content-string search so multi-line KiCAD format is handled correctly:
-                    b_depth = sum(1 for c in lines[i] if c == "(") - sum(
+            # KiCAD writes (symbol\n\t\t(lib_id "...") across two lines, which a
-                        1 for c in lines[i] if c == ")"
+            # line-by-line regex would never match.
-                    )
+            blocks_to_delete = []  # list of (char_start, char_end) into content
-                    j = i + 1
+            search_start = 0
-                    while j < len(lines) and b_depth > 0:
+            pattern = re.compile(r'\(symbol\s+\(lib_id\s+"')
-                        b_depth += sum(1 for c in lines[j] if c == "(") - sum(
+            while True:
-                            1 for c in lines[j] if c == ")"
+                m = pattern.search(content, search_start)
-                        )
+                if not m:
-                        j += 1
+                    break
-                    b_end = j - 1
+                pos = m.start()
-
+                # Skip blocks inside lib_symbols
-                    block_text = "\n".join(lines[b_start : b_end + 1])
+                if lib_sym_pos >= 0 and lib_sym_pos <= pos <= lib_sym_end:
-                    if re.search(
+                    search_start = lib_sym_end + 1
-                        r'\(property\s+"Reference"\s+"' + re.escape(reference) + r'"',
-                        block_text,
-                    ):
-                        blocks_to_delete.append((b_start, b_end))
-
-                    i = b_end + 1
                     continue
-
+                end = find_matching_paren(content, pos)
-                i += 1
+                if end < 0:
+                    search_start = pos + 1
+                    continue
+                block_text = content[pos : end + 1]
+                if re.search(
+                    r'\(property\s+"Reference"\s+"' + re.escape(reference) + r'"',
+                    block_text,
+                ):
+                    blocks_to_delete.append((pos, end))
+                search_start = end + 1
 
             if not blocks_to_delete:
                 return {
@@ -815,14 +811,18 @@ class KiCADInterface:
                     "message": f"Component '{reference}' not found in schematic (note: this tool removes schematic symbols, use delete_component for PCB footprints)",
                 }
 
-            # Delete from back to front to preserve line indices
+            # Delete from back to front to preserve character offsets
             for b_start, b_end in sorted(blocks_to_delete, reverse=True):
-                del lines[b_start : b_end + 1]
+                # Include any leading newline/whitespace before the block
-                if b_start < len(lines) and lines[b_start].strip() == "":
+                trim_start = b_start
-                    del lines[b_start]
+                while trim_start > 0 and content[trim_start - 1] in (" ", "\t"):
+                    trim_start -= 1
+                if trim_start > 0 and content[trim_start - 1] == "\n":
+                    trim_start -= 1
+                content = content[:trim_start] + content[b_end + 1:]
 
             with open(sch_file, "w", encoding="utf-8") as f:
-                f.write("\n".join(lines))
+                f.write(content)
 
             deleted_count = len(blocks_to_delete)
             logger.info(

python/tests/test_delete_schematic_component.py

@@ -0,0 +1,203 @@
+"""
+Regression tests for delete_schematic_component.
+
+Key regression: the handler previously used a line-by-line regex that required
+`(symbol` and `(lib_id` to appear on the *same* line.  KiCAD's file writer puts
+them on *separate* lines, so every real-world delete returned "not found".
+"""
+import re
+import sys
+import tempfile
+from pathlib import Path
+
+import pytest
+
+sys.path.insert(0, str(Path(__file__).parent.parent))
+
+TEMPLATE_SCH = Path(__file__).parent.parent / "templates" / "empty.kicad_sch"
+
+# Inline format (single line) – matches what tests previously used
+PLACED_RESISTOR_INLINE = '''\
+  (symbol (lib_id "Device:R") (at 50 50 0) (unit 1)
+    (in_bom yes) (on_board yes) (dnp no)
+    (uuid "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee")
+    (property "Reference" "R1" (at 51.27 47.46 0)
+      (effects (font (size 1.27 1.27)))
+    )
+    (property "Value" "10k" (at 51.27 52.54 0)
+      (effects (font (size 1.27 1.27)))
+    )
+    (property "Footprint" "Resistor_SMD:R_0603_1608Metric" (at 50 50 0)
+      (effects (font (size 1.27 1.27)) hide)
+    )
+    (property "Datasheet" "~" (at 50 50 0)
+      (effects (font (size 1.27 1.27)) hide)
+    )
+  )
+'''
+
+# Multi-line format – as KiCAD's own file writer produces it.
+# (symbol and (lib_id are on separate lines, which broke the old regex.
+PLACED_RESISTOR_MULTILINE = '''\
+\t(symbol
+\t\t(lib_id "Device:R")
+\t\t(at 50 50 0)
+\t\t(unit 1)
+\t\t(in_bom yes)
+\t\t(on_board yes)
+\t\t(dnp no)
+\t\t(uuid "bbbbbbbb-cccc-dddd-eeee-ffffffffffff")
+\t\t(property "Reference" "R2"
+\t\t\t(at 51.27 47.46 0)
+\t\t\t(effects
+\t\t\t\t(font
+\t\t\t\t\t(size 1.27 1.27)
+\t\t\t\t)
+\t\t\t)
+\t\t)
+\t\t(property "Value" "4.7k"
+\t\t\t(at 51.27 52.54 0)
+\t\t\t(effects
+\t\t\t\t(font
+\t\t\t\t\t(size 1.27 1.27)
+\t\t\t\t)
+\t\t\t)
+\t\t)
+\t)
+'''
+
+# Multi-line power symbol – the exact scenario that was reported as broken.
+PLACED_POWER_SYMBOL_MULTILINE = '''\
+\t(symbol
+\t\t(lib_id "power:VCC")
+\t\t(at 365.6 38.1 0)
+\t\t(unit 1)
+\t\t(in_bom yes)
+\t\t(on_board yes)
+\t\t(dnp no)
+\t\t(uuid "cccccccc-dddd-eeee-ffff-000000000030")
+\t\t(property "Reference" "#PWR030"
+\t\t\t(at 365.6 41.91 0)
+\t\t\t(effects
+\t\t\t\t(font
+\t\t\t\t\t(size 1.27 1.27)
+\t\t\t\t)
+\t\t\t\t(hide yes)
+\t\t\t)
+\t\t)
+\t\t(property "Value" "VCC"
+\t\t\t(at 365.6 35.56 0)
+\t\t\t(effects
+\t\t\t\t(font
+\t\t\t\t\t(size 1.27 1.27)
+\t\t\t\t)
+\t\t\t)
+\t\t)
+\t)
+'''
+
+
+def _make_test_schematic(tmp_path: Path, extra_block: str = "") -> Path:
+    dest = tmp_path / "test.kicad_sch"
+    src_content = TEMPLATE_SCH.read_text(encoding="utf-8")
+    if extra_block:
+        src_content = src_content.rstrip()
+        if src_content.endswith(")"):
+            src_content = src_content[:-1] + "\n" + extra_block + ")\n"
+    dest.write_text(src_content, encoding="utf-8")
+    return dest
+
+
+# ---------------------------------------------------------------------------
+# Unit tests – regression proof for the old regex vs the new approach
+# ---------------------------------------------------------------------------
+
+@pytest.mark.unit
+class TestDeleteDetectionRegex:
+    """Verify that the new content-string pattern finds blocks in both formats."""
+
+    OLD_PATTERN = re.compile(r"^\s*\(symbol\s+\(lib_id\s+\"", re.MULTILINE)
+    NEW_PATTERN = re.compile(r'\(symbol\s+\(lib_id\s+"')
+
+    def test_old_regex_fails_on_multiline_format(self):
+        """Regression: old line-by-line regex must NOT match the multi-line format."""
+        # The old code used re.match on individual lines; simulate that here.
+        lines = PLACED_RESISTOR_MULTILINE.split("\n")
+        matches = [l for l in lines if re.match(r"\s*\(symbol\s+\(lib_id\s+\"", l)]
+        assert matches == [], "Old regex should not match multi-line KiCAD format"
+
+    def test_old_regex_matches_inline_format(self):
+        """Old regex did work on single-line (inline) format."""
+        lines = PLACED_RESISTOR_INLINE.split("\n")
+        matches = [l for l in lines if re.match(r"\s*\(symbol\s+\(lib_id\s+\"", l)]
+        assert len(matches) == 1
+
+    def test_new_pattern_matches_multiline_format(self):
+        """New content-string pattern must find blocks in multi-line format."""
+        assert self.NEW_PATTERN.search(PLACED_RESISTOR_MULTILINE) is not None
+
+    def test_new_pattern_matches_inline_format(self):
+        """New content-string pattern also works on inline format."""
+        assert self.NEW_PATTERN.search(PLACED_RESISTOR_INLINE) is not None
+
+    def test_new_pattern_matches_power_symbol_multiline(self):
+        """New pattern must find #PWR030 power symbol in multi-line format."""
+        assert self.NEW_PATTERN.search(PLACED_POWER_SYMBOL_MULTILINE) is not None
+
+    def test_reference_extraction_from_multiline_block(self):
+        """Reference property can be found inside a multi-line block."""
+        ref_pattern = re.compile(r'\(property\s+"Reference"\s+"#PWR030"')
+        assert ref_pattern.search(PLACED_POWER_SYMBOL_MULTILINE) is not None
+
+
+# ---------------------------------------------------------------------------
+# Integration tests – real file I/O using the handler
+# ---------------------------------------------------------------------------
+
+@pytest.mark.integration
+class TestDeleteSchematicComponentIntegration:
+    def _get_handler(self):
+        from kicad_interface import KiCADInterface
+        iface = KiCADInterface.__new__(KiCADInterface)
+        return iface._handle_delete_schematic_component
+
+    def test_delete_inline_format_succeeds(self, tmp_path):
+        sch = _make_test_schematic(tmp_path, PLACED_RESISTOR_INLINE)
+        result = self._get_handler()({"schematicPath": str(sch), "reference": "R1"})
+        assert result["success"] is True
+        assert result["deleted_count"] == 1
+
+    def test_delete_multiline_format_succeeds(self, tmp_path):
+        """Regression: must succeed when KiCAD writes (symbol and (lib_id on separate lines."""
+        sch = _make_test_schematic(tmp_path, PLACED_RESISTOR_MULTILINE)
+        result = self._get_handler()({"schematicPath": str(sch), "reference": "R2"})
+        assert result["success"] is True
+        assert result["deleted_count"] == 1
+
+    def test_delete_power_symbol_multiline_succeeds(self, tmp_path):
+        """Regression: #PWR030 multi-line power symbol must be deletable."""
+        sch = _make_test_schematic(tmp_path, PLACED_POWER_SYMBOL_MULTILINE)
+        result = self._get_handler()({"schematicPath": str(sch), "reference": "#PWR030"})
+        assert result["success"] is True
+        assert result["deleted_count"] == 1
+
+    def test_component_absent_after_delete(self, tmp_path):
+        sch = _make_test_schematic(tmp_path, PLACED_POWER_SYMBOL_MULTILINE)
+        self._get_handler()({"schematicPath": str(sch), "reference": "#PWR030"})
+        remaining = sch.read_text(encoding="utf-8")
+        assert '"#PWR030"' not in remaining
+
+    def test_unknown_reference_returns_failure(self, tmp_path):
+        sch = _make_test_schematic(tmp_path, PLACED_RESISTOR_INLINE)
+        result = self._get_handler()({"schematicPath": str(sch), "reference": "U99"})
+        assert result["success"] is False
+        assert "not found" in result["message"]
+
+    def test_missing_schematic_path_returns_failure(self, tmp_path):
+        result = self._get_handler()({"reference": "R1"})
+        assert result["success"] is False
+
+    def test_missing_reference_returns_failure(self, tmp_path):
+        sch = _make_test_schematic(tmp_path)
+        result = self._get_handler()({"schematicPath": str(sch)})
+        assert result["success"] is False