feat: add kilm import command for SamacSys/Mouser/Ultralibrarian/SnapMagic ZIPs#9
Open
monkey265 wants to merge 8 commits into
Open
feat: add kilm import command for SamacSys/Mouser/Ultralibrarian/SnapMagic ZIPs#9monkey265 wants to merge 8 commits into
monkey265 wants to merge 8 commits into
Conversation
|
@monkey265 is attempting to deploy a commit to the Paxia LLC Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdds a ChangesImport ZIP feature
Estimated code review effort: 4 (Complex) | ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
tests/test_import_zip_command.py (1)
174-229: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing test coverage for the kicad-cli footprint upgrade path.
All three CLI integration tests patch
_detect_kicad_clito returnNone, so the kicad-cli-based legacy footprint upgrade path — a distinct feature described in the PR objectives — is never exercised end-to-end.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_import_zip_command.py` around lines 174 - 229, Add an end-to-end CLI integration test that exercises the kicad-cli footprint upgrade path in import_zip.command instead of patching _detect_kicad_cli to None, so the legacy footprint conversion flow is covered. Reuse the existing import/app runner setup and a Samacsys ZIP fixture, then assert the upgraded footprint is produced through the kicad-cli branch and the expected output reflects that path. Keep the current no-cli tests, but add a separate test that specifically validates the _detect_kicad_cli-driven behavior.Source: Coding guidelines
kicad_lib_manager/commands/import_zip/command.py (3)
139-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
kicad-clicommand-building logic.The
.appimagevs. plain-binary branching to buildcmdis duplicated almost verbatim between_upgrade_fpand_upgrade_sym. Extract a small helper, e.g._build_kicad_cli_cmd(kicad_cli: Path, *args: str) -> list[str], to avoid the two copies drifting.♻️ Proposed helper
+def _build_kicad_cli_cmd(kicad_cli: Path, *args: str) -> list[str]: + if kicad_cli.suffix.lower() == ".appimage": + return [str(kicad_cli), "kicad-cli", *args] + return [str(kicad_cli), *args]Also applies to: 171-174
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@kicad_lib_manager/commands/import_zip/command.py` around lines 139 - 159, The `cmd` construction in `_upgrade_fp` and `_upgrade_sym` is duplicated and should be centralized to avoid drift. Add a small helper such as `_build_kicad_cli_cmd` in `command.py` that takes `kicad_cli` plus the subcommand arguments and returns the correct command list for both `.appimage` and plain binaries, then replace both branching blocks with calls to that helper.
84-109: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftConsider a proper S-expression parser instead of text splicing.
_merge_symbolsinserts new symbol blocks by locating the last literal)in the whole file and splicing text around it. This works for well-formedkicad_symfiles but is fragile against any trailing content/formatting variance and offers no structural validation of the merged result. A KiCad-aware library (e.g.kiutils) would parse/serialize the S-expression tree and avoid this class of corruption risk for a file users rely on.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@kicad_lib_manager/commands/import_zip/command.py` around lines 84 - 109, The _merge_symbols function is currently doing unsafe text splicing by finding the last literal ")" in the kicad_sym file, which can corrupt formatting or structure. Refactor this flow to use a proper KiCad S-expression parser/serializer (for example via kiutils) when reading, merging, and writing symbols in _merge_symbols, so new blocks are inserted into the parsed tree and the result is structurally validated before saving.
274-277: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRedundant
shutil.whichcall and suppressed type error.
shutil.which("kicad-cli")is called twice (once in theif, once insidePath(...)), and the# type: ignore[arg-type]masks the fact thatshutil.whichreturnsOptional[str]. As per coding guidelines, code should use full explicit type hints and avoid dynamic-typing workarounds; capturing the result once avoids both issues.🔧 Proposed fix
- if shutil.which("kicad-cli"): - return Path(shutil.which("kicad-cli")) # type: ignore[arg-type] + found = shutil.which("kicad-cli") + if found: + return Path(found)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@kicad_lib_manager/commands/import_zip/command.py` around lines 274 - 277, The _detect_kicad_cli function redundantly calls shutil.which("kicad-cli") twice and relies on a suppressed type error to pass its result into Path. Update _detect_kicad_cli to store the result of shutil.which("kicad-cli") in a clearly typed local variable, check that variable once, and return Path(...) only when it is not None. This removes the duplicate lookup and the need for the type ignore while keeping the Optional[Path] behavior intact.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@kicad_lib_manager/commands/import_zip/command.py`:
- Around line 278-286: The fallback kicad-cli location list in the command
lookup logic is hardcoded inline and should be made configurable. Hoist the
paths used by the kicad-cli discovery function into a module-level constant
tuple in import_zip/command.py, and have the lookup helper iterate over that
constant instead of embedding literals directly; if appropriate, also allow an
environment variable override so additional paths like Windows installs can be
added without changing the function.
- Around line 381-396: The per-zip import path in the loop over zip_files
currently calls _import_zip without any exception handling, so failures like
ValueError or I/O errors will escape as a traceback instead of the existing
skip-and-log behavior. Wrap the _import_zip call in a try/except in the same
loop, catch import-related exceptions, and print a consistent red error message
that includes zip_path.name before continuing to the next archive, while leaving
the successful totals["sym"], totals["fp"], and totals["models"] aggregation
unchanged.
- Around line 125-176: The `_upgrade_fp` and `_upgrade_sym` helpers invoke
`subprocess.run` on `kicad-cli` without a timeout and ignore non-zero exits, so
a stuck or failed upgrade is silently swallowed. Update both helpers to call
`subprocess.run` with a reasonable `timeout=` and `check=True` or explicit
exit-code handling, and catch `subprocess.TimeoutExpired` plus command failures
to log the stderr/stdout details. Keep the handling localized in `_upgrade_fp`,
`_upgrade_sym`, and the shared `kicad_cli` invocation path so import continues
safely while surfacing the failure.
- Around line 181-195: The containment check in _safe_extractall currently
relies on string prefix matching, which breaks on Windows because Path string
separators differ; replace it with a path-aware relative check against
dest_resolved so valid ZIP entries are accepted across platforms. Also update
the _safe_extractall docstring to describe this as defense-in-depth rather than
implying zipfile.extractall() only recently gained sanitization in Python 3.12,
while keeping the unsafe-entry rejection behavior unchanged.
In `@tests/test_import_zip_command.py`:
- Around line 46-51: Add explicit return annotations to every test_* function in
this file so they consistently use -> None like the existing typed helpers and
fixtures. Update the affected test functions such as test_extract_symbol_blocks
and the other test cases referenced in this file to include a None return type,
keeping the rest of the test logic unchanged and preserving strict
pyrefly-compatible typing.
---
Nitpick comments:
In `@kicad_lib_manager/commands/import_zip/command.py`:
- Around line 139-159: The `cmd` construction in `_upgrade_fp` and
`_upgrade_sym` is duplicated and should be centralized to avoid drift. Add a
small helper such as `_build_kicad_cli_cmd` in `command.py` that takes
`kicad_cli` plus the subcommand arguments and returns the correct command list
for both `.appimage` and plain binaries, then replace both branching blocks with
calls to that helper.
- Around line 84-109: The _merge_symbols function is currently doing unsafe text
splicing by finding the last literal ")" in the kicad_sym file, which can
corrupt formatting or structure. Refactor this flow to use a proper KiCad
S-expression parser/serializer (for example via kiutils) when reading, merging,
and writing symbols in _merge_symbols, so new blocks are inserted into the
parsed tree and the result is structurally validated before saving.
- Around line 274-277: The _detect_kicad_cli function redundantly calls
shutil.which("kicad-cli") twice and relies on a suppressed type error to pass
its result into Path. Update _detect_kicad_cli to store the result of
shutil.which("kicad-cli") in a clearly typed local variable, check that variable
once, and return Path(...) only when it is not None. This removes the duplicate
lookup and the need for the type ignore while keeping the Optional[Path]
behavior intact.
In `@tests/test_import_zip_command.py`:
- Around line 174-229: Add an end-to-end CLI integration test that exercises the
kicad-cli footprint upgrade path in import_zip.command instead of patching
_detect_kicad_cli to None, so the legacy footprint conversion flow is covered.
Reuse the existing import/app runner setup and a Samacsys ZIP fixture, then
assert the upgraded footprint is produced through the kicad-cli branch and the
expected output reflects that path. Keep the current no-cli tests, but add a
separate test that specifically validates the _detect_kicad_cli-driven behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 58c31866-df41-45b0-b0b9-8f8f5776e581
📒 Files selected for processing (4)
kicad_lib_manager/commands/import_zip/__init__.pykicad_lib_manager/commands/import_zip/command.pykicad_lib_manager/main.pytests/test_import_zip_command.py
…ows, and per-zip errors
Author
|
New commit fixes all CodeRabbit suggestions. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@kicad_lib_manager/commands/import_zip/command.py`:
- Around line 141-148: The warning output in the import ZIP upgrade path is
printing raw bytes because `subprocess.run` in the upgrade helper(s) under
`command.py` uses `capture_output=True` without text decoding. Update the
relevant `subprocess.run` call(s) so `exc.stderr` is handled as text, either by
enabling `text=True`/`universal_newlines=True` or by decoding before formatting,
and keep the `except subprocess.CalledProcessError` message in the same `fp
upgrade` flow readable for `path.name`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 31621876-079c-47f6-8d25-86cd644f4dca
📒 Files selected for processing (1)
kicad_lib_manager/commands/import_zip/command.py
Author
|
Fixed subprocess stderr decoding: added |
UltraLibrarian exports differ from SamacSys/Mouser: the KiCad dir is named "KiCADv6" (case differs, prior lookup was case-sensitive), footprints nest under a "*.pretty" subdir, and symbol files use 2-space/CRLF formatting instead of tabs. Match KiCad/3D dirs case-insensitively, glob footprints/symbols recursively, and rewrite symbol-block extraction to track paren depth instead of relying on a tab-indentation regex, so it works regardless of generator whitespace style. Verified against a real UltraLibrarian export. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Author
|
Also added support for ultralibrarian |
monkey265
changed the title
|
Awesome idea! This would really help my workflow |
SnapMagic exports have no wrapping "KiCad"/"3D" directory at all - the .kicad_sym, .kicad_mod, and .step files sit flat at the ZIP root. Drop the vendor-specific directory-name matching (case-insensitive "kicad"/ "3d" dirs) in favor of searching the whole extracted tree by file extension, which also still covers SamacSys/Mouser and UltraLibrarian layouts. Verified against a real SnapMagic export. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Two sequential re.sub passes let the second (unquoted-path) regex re-match inside the first pass's already-substituted output, and the extension alternation tried "step"/"stp" before their compound forms "step.gz"/"stp.gz" (which contain them as literal prefixes) - both issues caused any gzipped 3D model reference to be truncated mid extension, leaving a dangling ".gz" and a malformed quoted string in the rewritten footprint file. Replaced with a single combined regex with compound extensions ordered first.
- Reformat with black (command.py, test file were failing black --check despite passing ruff, since E501 is delegated to black) - Sync "kilm import" help text across __init__.py/main.py with the vendor list already in the command docstring (UltraLibrarian, SnapMagic) - Pin COLUMNS=200 in tests so Rich's terminal-width auto-detection can't wrap output differently across environments and break substring assertions
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_import_zip_command.py (1)
86-86: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd
-> Nonereturn type hints to test functions.This was previously flagged and remains unresolved. The newly added test functions
test_fix_3d_path_handles_all_extensions_without_truncation(Line 118) andtest_import_zip_cli_adds_snapmagic_part(Line 330) also omit the annotation, as doestest_safe_extractall_rejects_zip_slip(Line 86).As per coding guidelines, "Use full, explicit type hints everywhere; do not use Any or dynamic typing" for
**/*.py.🔧 Apply `-> None` to all test functions
-def test_safe_extractall_rejects_zip_slip(tmp_path: Path): +def test_safe_extractall_rejects_zip_slip(tmp_path: Path) -> None:-def test_fix_3d_path_handles_all_extensions_without_truncation(raw: str): +def test_fix_3d_path_handles_all_extensions_without_truncation(raw: str) -> None:-def test_import_zip_cli_adds_snapmagic_part( - tmp_path: Path, library_tree: Path, mock_config: MagicMock -): +def test_import_zip_cli_adds_snapmagic_part( + tmp_path: Path, library_tree: Path, mock_config: MagicMock +) -> None:Also applies to: 118-118, 330-330
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_import_zip_command.py` at line 86, Add explicit -> None return type annotations to the test functions test_safe_extractall_rejects_zip_slip, test_fix_3d_path_handles_all_extensions_without_truncation, and test_import_zip_cli_adds_snapmagic_part.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/test_import_zip_command.py`:
- Line 86: Add explicit -> None return type annotations to the test functions
test_safe_extractall_rejects_zip_slip,
test_fix_3d_path_handles_all_extensions_without_truncation, and
test_import_zip_cli_adds_snapmagic_part.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3ecc30e3-6fa0-4e9f-8292-71dd055de381
📒 Files selected for processing (4)
kicad_lib_manager/commands/import_zip/__init__.pykicad_lib_manager/commands/import_zip/command.pykicad_lib_manager/main.pytests/test_import_zip_command.py
🚧 Files skipped from review as they are similar to previous changes (2)
- kicad_lib_manager/commands/import_zip/init.py
- kicad_lib_manager/main.py
monkey265
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Added
kilm importcommand that imports KiCad component ZIPs downloaded from Mouser or SamacSys directly into the configured library..stp/.step) →LIB.3dshapes/(module ...)footprints to current format via kicad-cli (optional, skipped gracefully if not found)symbols/LIB.kicad_symwith correctLIB:footprint prefixTest plan
black,ruff check,pytestall passSummary by CodeRabbit
New Features
kilm importCLI subcommand to import SamacSys/Mouser/UltraLibrarian/SnapMagic KiCad ZIPs into your library.--dry-runpreview.Bug Fixes
${KICAD_3RD_PARTY}3D model normalization).Tests
dry_runvs. real writes.