diff --git a/python/kicad_interface.py b/python/kicad_interface.py index 50915cf..84580c0 100644 --- a/python/kicad_interface.py +++ b/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 - lib_sym_start, lib_sym_end = None, None - depth = 0 - for i, line in enumerate(lines): - if "(lib_symbols" in line and lib_sym_start is None: - lib_sym_start = i - depth = sum(1 for c in line if c == "(") - sum( - 1 for c in line if c == ")" - ) - elif lib_sym_start is not None and lib_sym_end is None: - depth += sum(1 for c in line if c == "(") - sum( - 1 for c in line if c == ")" - ) - if depth == 0: - lib_sym_end = i - break + def find_matching_paren(s, start): + """Find the closing paren matching the opening paren at start.""" + depth = 0 + i = start + while i < len(s): + if s[i] == "(": + depth += 1 + elif s[i] == ")": + depth -= 1 + if depth == 0: + return i + i += 1 + return -1 - # Find ALL placed symbol blocks matching the reference (handles duplicates) - blocks_to_delete = [] - i = 0 - while i < len(lines): - # 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 + # Skip lib_symbols section + lib_sym_pos = content.find("(lib_symbols") + lib_sym_end = ( + find_matching_paren(content, lib_sym_pos) if lib_sym_pos >= 0 else -1 + ) - if re.match(r"\s*\(symbol\s+\(lib_id\s+\"", lines[i]): - b_start = i - b_depth = sum(1 for c in lines[i] if c == "(") - sum( - 1 for c in lines[i] if c == ")" - ) - j = i + 1 - while j < len(lines) and b_depth > 0: - b_depth += sum(1 for c in lines[j] if c == "(") - sum( - 1 for c in lines[j] if c == ")" - ) - j += 1 - b_end = j - 1 - - block_text = "\n".join(lines[b_start : b_end + 1]) - if re.search( - r'\(property\s+"Reference"\s+"' + re.escape(reference) + r'"', - block_text, - ): - blocks_to_delete.append((b_start, b_end)) - - i = b_end + 1 + # Find ALL placed symbol blocks matching the reference (handles duplicates). + # Use content-string search so multi-line KiCAD format is handled correctly: + # KiCAD writes (symbol\n\t\t(lib_id "...") across two lines, which a + # line-by-line regex would never match. + blocks_to_delete = [] # list of (char_start, char_end) into content + search_start = 0 + pattern = re.compile(r'\(symbol\s+\(lib_id\s+"') + while True: + m = pattern.search(content, search_start) + if not m: + break + pos = m.start() + # Skip blocks inside lib_symbols + if lib_sym_pos >= 0 and lib_sym_pos <= pos <= lib_sym_end: + search_start = lib_sym_end + 1 continue - - i += 1 + end = find_matching_paren(content, pos) + 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] - if b_start < len(lines) and lines[b_start].strip() == "": - del lines[b_start] + # Include any leading newline/whitespace before the block + trim_start = 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( diff --git a/python/tests/test_delete_schematic_component.py b/python/tests/test_delete_schematic_component.py new file mode 100644 index 0000000..528d033 --- /dev/null +++ b/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