feat: add kilm import command for SamacSys/Mouser/Ultralibrarian ZIPs#9
Open
monkey265 wants to merge 5 commits into
Open
feat: add kilm import command for SamacSys/Mouser/Ultralibrarian ZIPs#9monkey265 wants to merge 5 commits into
monkey265 wants to merge 5 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 | ✅ 5✅ Passed checks (5 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 |
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
--dry-runpreview and detailed add/skip reporting.Bug Fixes
${KICAD_3RD_PARTY}layout.Tests