Feature/self update #5
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughIntroduces a new sync command for updating Git-based libraries, repurposes update to self-update KiLM via multiple installation methods, adds auto-update/version-check infrastructure with caching, updates config for update-check preferences, revises docs and navigation accordingly, bumps version to 0.4.0 with new dependencies, updates hooks and tests, and adds pre-commit hooks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as kilm CLI
participant Sync as sync command
participant FS as Filesystem
participant Git as git
participant Setup as setup command
User->>CLI: kilm sync [--dry-run|--verbose|--auto-setup]
CLI->>Sync: invoke
Sync->>FS: load config (filter type="github")
alt no libraries
Sync-->>User: "No GitHub libraries configured"
else libraries present
loop for each library
Sync->>FS: validate path + .git
alt --dry-run
Sync-->>User: "Dry run: would run git pull"
else
Sync->>Git: git pull (cwd=lib)
alt updated
Sync->>Git: git diff --name-only
Sync->>FS: scan for .kicad_sym/.pretty/templates
Sync-->>User: report changes
opt --auto-setup
Sync->>Setup: invoke kilm setup
Setup-->>Sync: result
end
else up-to-date
Sync-->>User: report up-to-date
end
end
end
Sync-->>User: summary counts and next steps
end
sequenceDiagram
autonumber
actor User
participant CLI as kilm CLI
participant Update as update command
participant Cfg as Config
participant UM as UpdateManager
participant PyPI as PyPI API
participant Shell as System
User->>CLI: kilm update [--check|--force]
CLI->>Update: invoke
Update->>Cfg: read update preferences
Update->>UM: init(current_version)
UM->>PyPI: GET /pypi/kilm/json (If-None-Match: ETag)
alt 304 Not Modified
UM-->>Update: latest from cache
else 200 OK
UM-->>Update: latest version
else error
UM-->>Update: None
end
alt --check or no update
Update-->>User: report status & manual instruction
else update available
alt UM.can_auto_update() true
Update->>Shell: run pipx/pip/uv update command
alt success
Update-->>User: success & new version
else failure
Update-->>User: error + manual instruction
end
else
Update-->>User: manual update instruction (conda/homebrew)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pyproject.toml (1)
45-52: Drop third-party pathlib on Python ≥3.8 to avoid stdlib shadowing.Depending on the backport can cause import conflicts on modern Python.
dependencies = [ "click>=8.0", "pyyaml>=6.0", - "pathlib>=1.0.1", "pathspec>=0.12.1", "jinja2>=3.1.6", "questionary>=1.10.0", ]Also applies to: 48-48
kicad_lib_manager/config.py (1)
28-36: Type accuracy: include bools in ConfigValueConfig stores booleans, but ConfigValue excludes bool. This causes type-check noise and unsafe casts.
-ConfigValue = Union[str, int, List[LibraryDict]] +ConfigValue = Union[str, int, bool, List[LibraryDict]]
🧹 Nitpick comments (29)
pyproject.toml (1)
18-35: Remove duplicate classifier entry."Development Status :: 4 - Beta" appears twice; keep a single instance.
"Framework :: Pytest", - "Development Status :: 4 - Beta", "Natural Language :: English", "Environment :: Console",Also applies to: 32-32
kicad_lib_manager/__init__.py (1)
5-5: Avoid duplicating the version; read from package metadata.Keeps pyproject.toml as single source of truth (noting your docs-site versioning is intentionally separate).
-__version__ = "0.4.0" +from importlib.metadata import PackageNotFoundError, version as _version +try: + __version__ = _version("kilm") +except PackageNotFoundError: + __version__ = "0.0.0+unknown"docs/src/content/docs/guides/troubleshooting.mdx (1)
53-56: Clarify self-update path alongside sync.Since update now means self-update, add a pointer here.
- Use the `--verbose` flag with commands like `kilm setup` or `kilm sync` to get more detailed output. +- To upgrade KiLM itself, run `kilm update`.kicad_lib_manager/utils/git_utils.py (2)
74-86: Preserve original encoding and mode when backing up hooksReading/writing as UTF-8 may fail on non-UTF8 hooks, and forcing 0755 broadens permissions. Copy bytes and mirror the original mode.
- # Copy the file content - backup_path.write_text(hook_path.read_text(encoding="utf-8")) - - # Preserve executable permissions (Unix-like systems only) - if ( - os.name != "nt" and hook_path.stat().st_mode & 0o111 - ): # Not Windows and executable - backup_path.chmod(0o755) + # Copy bytes to preserve encoding; mirror original mode + data = hook_path.read_bytes() + backup_path.write_bytes(data) + try: + # Mirror full permission bits on POSIX + if os.name != "nt": + backup_path.chmod(hook_path.stat().st_mode & 0o777) + except OSError: + pass
100-123: Handle partial/duplicate KiLM sections more defensivelyIf only one marker exists, current logic appends a new section, leaving a dangling/partial old section. Consider stripping any partial section first.
tests/test_git_utils.py (1)
27-27: Tests updated to “kilm sync” — add assertions for header and completion messageStrengthen coverage to catch regressions in hook header and final echo.
- assert "kilm sync" in content + assert "kilm sync" in content + assert "KiCad Library Manager auto-sync hook" in content + assert "KiCad libraries sync complete." in content @@ - assert "kilm sync" in result + assert "kilm sync" in result + assert "KiCad Library Manager auto-sync hook" in result + assert "KiCad libraries sync complete." in resultAlso applies to: 80-81
kicad_lib_manager/commands/add_hook/docs.mdx (3)
79-92: Align header and completion text with “sync” terminologyHeader still says “auto-update” and closing message says “update complete.” Update for consistency.
#!/bin/sh # BEGIN KiLM-managed section -# KiCad Library Manager auto-update hook +# KiCad Library Manager auto-sync hook # Added by kilm add-hook command @@ -echo "KiCad libraries update complete." +echo "KiCad libraries sync complete." # END KiLM-managed section
121-135: Also fix the customization example to use “auto-sync” and “sync complete”Keep examples consistent with the new command.
#!/bin/sh # BEGIN KiLM-managed section -# KiCad Library Manager auto-update hook +# KiCad Library Manager auto-sync hook # Added by kilm add-hook command @@ -echo "KiCad libraries update complete." +echo "KiCad libraries sync complete." # END KiLM-managed section
1-4: Optional: frontmatter description can say “synchronize”Minor wording tweak for accuracy.
-description: Add a Git post-merge hook to automatically update KiLM-managed libraries. +description: Add a Git post-merge hook to automatically synchronize KiLM-managed libraries.docs/src/content/docs/guides/automatic-updates.mdx (3)
23-29: Fix list formatting in Aside for readabilityRender items as proper bullets.
-<Aside type="tip" title="Enhanced Hook Management"> - The `kilm add-hook` command now includes advanced features: - **Smart - Detection:** Automatically finds the correct hooks directory (supports custom - `core.hooksPath` and Git worktrees) - **Safe Updates:** Creates backups and - intelligently merges with existing hooks - **Idempotent:** Safe to run - multiple times without duplicating content - **Clear Markers:** Uses section - markers to identify KiLM-managed content -</Aside> +<Aside type="tip" title="Enhanced Hook Management"> + The `kilm add-hook` command includes: + - **Smart detection:** Supports custom `core.hooksPath` and Git worktrees + - **Safe updates:** Creates backups and intelligently merges with existing hooks + - **Idempotence:** Safe to run multiple times without duplicating content + - **Clear markers:** Uses section markers to identify KiLM-managed content +</Aside>
49-62: Update sample header comment to “auto-sync hook”Keep the sample consistent.
#!/bin/sh # BEGIN KiLM-managed section -# KiCad Library Manager auto-update hook +# KiCad Library Manager auto-sync hook # Added manually @@ -echo "KiCad libraries sync complete." +echo "KiCad libraries sync complete."
75-76: Wording nit: “check for library updates” → “sync libraries”Optional clarity tweak.
-2. The hook runs `kilm sync` to check for library updates +2. The hook runs `kilm sync` to synchronize librarieskicad_lib_manager/commands/sync/docs.mdx (2)
28-31: “update process” → “sync process”Match command naming.
- Show detailed output during the update process, including the full output from the `git pull` commands. + Show detailed output during the sync process, including the full output from the `git pull` commands.
22-38: Tighten options list formattingMinor formatting cleanup; remove stray hyphen and keep consistent bullets.
- `--dry-run`: Show which libraries are detected as Git repositories and would be updated, but do not actually run `git pull`. _Example:_ `kilm sync --dry-run` - `--verbose`: Show detailed output during the sync process, including the full output from the `git pull` commands. _Example:_ `kilm sync --verbose` - `--auto-setup`: If the `git pull` operation results in changes that likely require updating KiCad's configuration (e.g., new symbol or footprint libraries detected), automatically run `kilm setup` after the updates are complete. Default: `False`. _Example:_ `kilm sync --auto-setup` -- `--help`: +- `--help`: Show the help message and exit.kicad_lib_manager/commands/update/docs.mdx (4)
8-16: Breaking-change copy: tighten and clarify wordingSmall copyedits for clarity and tone.
-:::caution[Breaking Change in v0.4.0] -The `kilm update` command behavior changed in KiLM v0.4.0: +:::caution[Breaking change in v0.4.0] +The behavior of `kilm update` changed in KiLM v0.4.0: - **Now:** Updates KiLM itself (self-update) - **Previously:** Updated library content (now use `kilm sync`) -A deprecation banner will guide users during the transition period. +KiLM shows a deprecation banner during the transition period. :::
39-43: Behavior list: reflect venv explicitlyDocs mention “pip (venv)” later; include it here for consistency.
-1. **Installation Detection:** Automatically detects how KiLM was installed (pipx, pip, conda, uv, homebrew). +1. **Installation Detection:** Automatically detects how KiLM was installed (pipx, pip, pip (venv), uv, conda, homebrew).
89-92: Permission error tip: include PATH note for --user installsUsers often miss adding ~/.local/bin to PATH after --user. Add one line.
-**Update fails with permission errors:** -- For system pip: Try `pip install --user --upgrade kilm` +**Update fails with permission errors:** +- For system pip: Try `pip install --user --upgrade kilm` (ensure `~/.local/bin` is on your PATH)
75-85: Config keys: align terms with CLI and valuesMinor polish: quote values and align wording.
-# Set check frequency -kilm config set update_check_frequency weekly # daily, weekly, never +# Set check frequency +kilm config set update_check_frequency "weekly" # one of: "daily", "weekly", "never"kicad_lib_manager/auto_update.py (3)
64-101: Cache/HTTP handling: fallback on 429 with cached dataMinor resilience: on 429, return cached version if available.
- if response.status_code == 200: + if response.status_code == 200: data = response.json() @@ - except (requests.RequestException, KeyError, json.JSONDecodeError): + if response.status_code == 429 and cached_data: + return cached_data.get("version") + except (requests.RequestException, KeyError, json.JSONDecodeError): # Fail silently - don't block CLI functionality pass
191-202: Instruction fallback: provide a concrete default“Check your package manager” is vague. Default to a safe pip command.
- return instructions.get(self.installation_method, "Check your package manager") + return instructions.get(self.installation_method, f"{sys.executable} -m pip install --upgrade kilm")
207-237: Surface stderr/stdout on failures (optional)Today failures return a generic message, hiding useful error output. Consider returning the captured stderr tail.
If acceptable, change update_via_* to return (success: bool, detail: str) and thread messages through perform_update. I can draft the refactor on request.
kicad_lib_manager/commands/add_hook/command.py (2)
1-4: Align wording with “sync” everywhere and write hook in UTF-8A couple of user-facing strings still say “update”; also specify encoding when writing the hook for consistency with the UTF-8 read.
Apply:
@@ -Adds a git post-merge hook to the current repository to automatically update KiCad libraries. +Adds a git post-merge hook to the current repository to automatically sync KiCad libraries. @@ - with post_merge_hook.open("w") as f: + with post_merge_hook.open("w", encoding="utf-8") as f: f.write(new_content) @@ - "The hook will run 'kilm sync' after every 'git pull' or 'git merge' operation." + "The hook will run 'kilm sync' after every 'git pull' or 'git merge' operation." @@ - click.echo("the update behavior or automatically set up libraries.") + click.echo("the sync behavior or automatically set up libraries.")Also applies to: 33-38, 93-95, 101-102, 111-114
56-57: Consider supporting rebase/FF workflows, not just post-mergePost-merge won’t fire on fast-forward pulls or rebases. Optionally install analogous post-checkout/post-rewrite hooks or document the limitation.
tests/test_sync_command.py (3)
110-121: Use a real module for sys.modules injection to avoid importlib edge casesUsing ModuleType is safer than a bare Mock for module injection.
- with patch("click.Context", return_value=context_mock): - # Mock the setup module import - setup_module_mock = Mock() - setup_module_mock.setup = Mock(name="setup_command") - # Mock the module import - with patch.dict( - "sys.modules", {"kicad_lib_manager.commands.setup": setup_module_mock} - ): + with patch("click.Context", return_value=context_mock): + import types + setup_mod = types.ModuleType("kicad_lib_manager.commands.setup") + setup_mod.setup = Mock(name="setup_command") + with patch.dict("sys.modules", {"kicad_lib_manager.commands.setup": setup_mod}): runner = CliRunner() result = runner.invoke(main, ["sync", "--auto-setup"])
58-61: Avoid monkeypatching Path.truedivOverriding special methods on pathlib.Path can have surprising side effects. Prefer leaving Path behavior intact and only customizing MagicMock instances used in template checks.
Also applies to: 83-84
63-79: Template detection mock may over-matchThe branch keyed on '' risks always producing a “templates” hit. Tighten the condition to pattern == "" and keep the metadata.yaml check within the returned dir.
- elif "*" in pattern and "metadata.yaml" not in str(self): + elif pattern == "*": mock_dir = MagicMock() mock_dir.name = "template-dir" mock_dir.__truediv__ = lambda self, other: Path(f"{self}/{other}") mock_dir.exists = lambda: True return [mock_dir]kicad_lib_manager/commands/sync/command.py (2)
98-105: Add timeout and safer pull modeWithout a timeout, git can hang (e.g., network stalls). Also prefer fast-forward only to avoid unintended merges.
- result = subprocess.run( - ["git", "pull"], + result = subprocess.run( + ["git", "pull", "--ff-only"], cwd=lib_path, capture_output=True, text=True, check=False, + timeout=60, )
59-63: Dry-run summary should not claim real syncDry-run currently increments “updated” and the summary says “synced”. Track “would sync” separately and adjust the summary text.
- updated_count = 0 # Actually changed - up_to_date_count = 0 # Already at latest version + updated_count = 0 # Actually changed + up_to_date_count = 0 # Already at latest version skipped_count = 0 # Could not sync (not git, etc.) failed_count = 0 # Git pull failed + would_sync_count = 0 # Dry-run: would sync @@ if dry_run: click.echo(f" Dry run: would execute 'git pull' in {lib_path}") - updated_count += 1 + would_sync_count += 1 continue @@ - click.echo("\nSync Summary:") - click.echo(f" {updated_count} libraries synced") + click.echo("\nSync Summary:") + if dry_run: + click.echo(f" {would_sync_count} libraries would be synced") + else: + click.echo(f" {updated_count} libraries synced") click.echo(f" {up_to_date_count} libraries up to date") click.echo(f" {skipped_count} libraries skipped") click.echo(f" {failed_count} libraries failed")Also applies to: 91-95, 141-146
kicad_lib_manager/commands/update/command.py (1)
71-78: Optional: add exit codes for --check to aid CI scriptingReturn non-zero when an update is available or when the check fails.
- if check: - if update_manager.is_newer_version_available(latest_version): - click.echo(f"\nUpdate available: {latest_version}") - click.echo(f"To update, run: {update_manager.get_update_instruction()}") - else: - click.echo("No updates available.") - return + if check: + newer = update_manager.is_newer_version_available(latest_version) + if newer: + click.echo(f"\nUpdate available: {latest_version}") + click.echo(f"To update, run: {update_manager.get_update_instruction()}") + click.get_current_context().exit(10) # Non-zero signals update available + else: + click.echo("No updates available.") + click.get_current_context().exit(0)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (20)
docs/astro.config.mjs(1 hunks)docs/src/content/docs/guides/automatic-updates.mdx(4 hunks)docs/src/content/docs/guides/troubleshooting.mdx(1 hunks)docs/src/content/docs/reference/cli/index.mdx(1 hunks)kicad_lib_manager/__init__.py(1 hunks)kicad_lib_manager/auto_update.py(1 hunks)kicad_lib_manager/cli.py(2 hunks)kicad_lib_manager/commands/add_hook/command.py(2 hunks)kicad_lib_manager/commands/add_hook/docs.mdx(4 hunks)kicad_lib_manager/commands/sync/__init__.py(1 hunks)kicad_lib_manager/commands/sync/command.py(1 hunks)kicad_lib_manager/commands/sync/docs.mdx(1 hunks)kicad_lib_manager/commands/update/__init__.py(1 hunks)kicad_lib_manager/commands/update/command.py(1 hunks)kicad_lib_manager/commands/update/docs.mdx(2 hunks)kicad_lib_manager/config.py(3 hunks)kicad_lib_manager/utils/git_utils.py(1 hunks)pyproject.toml(1 hunks)tests/test_git_utils.py(2 hunks)tests/test_sync_command.py(9 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-05T07:17:34.953Z
Learnt from: barisgit
PR: barisgit/KiLM#3
File: kicad_lib_manager/__init__.py:5-5
Timestamp: 2025-09-05T07:17:34.953Z
Learning: In the KiLM project, different version numbers are intentionally maintained separately: the main Python package version in kicad_lib_manager/__init__.py, the docs site version in docs/package.json, and example versions in documentation files are not meant to be synchronized.
Applied to files:
pyproject.tomlkicad_lib_manager/__init__.pykicad_lib_manager/commands/update/docs.mdx
🧬 Code graph analysis (9)
docs/src/content/docs/reference/cli/index.mdx (1)
tests/test_update_command.py (3)
test_update_command(87-101)test_update_dry_run(158-167)test_update_with_already_up_to_date(130-146)
kicad_lib_manager/commands/sync/command.py (3)
kicad_lib_manager/config.py (3)
Config(39-401)get_libraries(141-156)get(70-74)kicad_lib_manager/commands/setup/command.py (1)
setup(125-527)tests/test_update_command.py (1)
test_check_for_library_changes(185-213)
kicad_lib_manager/commands/update/__init__.py (2)
kicad_lib_manager/commands/update/command.py (1)
update(27-94)tests/test_update_command.py (2)
test_check_for_library_changes(185-213)test_update_command(87-101)
kicad_lib_manager/commands/sync/docs.mdx (1)
docs/scripts/sync-embedded-docs.ts (3)
syncDoc(147-163)syncAllDocs(168-188)watchDocs(193-264)
docs/astro.config.mjs (1)
docs/scripts/sync-embedded-docs.ts (9)
syncDoc(147-163)watchDocs(193-264)CommandDoc(30-34)syncAllDocs(168-188)filePath(215-231)findCommandDocs(60-101)processEmbeddedDoc(106-142)cleanupDocs(269-289)dirPath(240-245)
tests/test_sync_command.py (3)
kicad_lib_manager/commands/sync/command.py (2)
sync(37-176)check_for_library_changes(180-238)kicad_lib_manager/config.py (1)
get_libraries(141-156)tests/test_update_command.py (1)
test_update_command(87-101)
kicad_lib_manager/cli.py (2)
kicad_lib_manager/commands/sync/command.py (1)
sync(37-176)kicad_lib_manager/commands/setup/command.py (1)
setup(125-527)
kicad_lib_manager/commands/sync/__init__.py (2)
kicad_lib_manager/commands/sync/command.py (1)
sync(37-176)kicad_lib_manager/commands/setup/command.py (1)
setup(125-527)
kicad_lib_manager/commands/update/command.py (1)
kicad_lib_manager/auto_update.py (7)
UpdateManager(163-236)check_latest_version(64-101)check_latest_version(171-173)is_newer_version_available(175-189)get_update_instruction(191-201)can_auto_update(203-205)perform_update(207-236)
🪛 LanguageTool
kicad_lib_manager/commands/sync/docs.mdx
[grammar] ~29-~29: There might be a mistake here.
Context: ...ull output from the git pull commands. Example: kilm sync --verbose - `-...
(QB_NEW_EN)
[grammar] ~36-~36: There might be a mistake here.
Context: ...:_ kilm sync --auto-setup - --help: Show the help message and exit. ## Beha...
(QB_NEW_EN)
[grammar] ~76-~76: There might be a mistake here.
Context: ...Git pull fails with merge conflicts: Navigate to the library directory and re...
(QB_NEW_EN)
[grammar] ~85-~85: There might be a mistake here.
Context: ...brary not found or not a Git repository:** - Ensure the library path is correct in yo...
(QB_NEW_EN)
[grammar] ~90-~90: There might be a mistake here.
Context: ...e libraries Permission denied errors: - Check file permissions in the library di...
(QB_NEW_EN)
kicad_lib_manager/commands/update/docs.mdx
[grammar] ~10-~10: There might be a mistake here.
Context: .... :::caution[Breaking Change in v0.4.0] The kilm update command behavior chang...
(QB_NEW_EN)
[grammar] ~11-~11: There might be a mistake here.
Context: ...command behavior changed in KiLM v0.4.0: - Now: Updates KiLM itself (self-update)...
(QB_NEW_EN)
[grammar] ~12-~12: There might be a mistake here.
Context: ...Now:** Updates KiLM itself (self-update) - Previously: Updated library content (n...
(QB_NEW_EN)
[grammar] ~34-~34: There might be a mistake here.
Context: ...ple:_ kilm update --force - --help: Show the help message and exit. ## Beha...
(QB_NEW_EN)
[grammar] ~40-~40: There might be a mistake here.
Context: ...available version using cached requests. 3. Update Execution: For supported method...
(QB_NEW_EN)
[grammar] ~46-~46: There might be a mistake here.
Context: ... Method | Auto-Update | Update Command | |--------|-------------|----------------...
(QB_NEW_EN)
[grammar] ~47-~47: There might be a mistake here.
Context: ...--------|-------------|----------------| | pipx ✅ | Yes | pipx upgrade kilm...
(QB_NEW_EN)
[grammar] ~48-~48: There might be a mistake here.
Context: ...pipx ✅ | Yes | pipx upgrade kilm | | pip ✅ | Yes | `pip install --upgra...
(QB_NEW_EN)
[grammar] ~49-~49: There might be a mistake here.
Context: ...✅ | Yes | pip install --upgrade kilm | | pip (venv) ✅ | Yes | `pip install ...
(QB_NEW_EN)
[grammar] ~50-~50: There might be a mistake here.
Context: ...✅ | Yes | pip install --upgrade kilm | | uv ✅ | Yes | `uv tool upgrade kilm...
(QB_NEW_EN)
[grammar] ~51-~51: There might be a mistake here.
Context: ...uv* ✅ | Yes | uv tool upgrade kilm | | conda | Manual | `conda update kil...
(QB_NEW_EN)
[grammar] ~52-~52: There might be a mistake here.
Context: ...conda** | Manual | conda update kilm | | homebrew | Manual | `brew upgrade ...
(QB_NEW_EN)
[grammar] ~63-~63: There might be a mistake here.
Context: ... kilm update --check **Update KiLM to latest version:** bash kilm update ...
(QB_NEW_EN)
[grammar] ~89-~89: There might be a mistake here.
Context: ... Update fails with permission errors: - For system pip: Try `pip install --user ...
(QB_NEW_EN)
[grammar] ~90-~90: There might be a mistake here.
Context: ...mission errors:** - For system pip: Try pip install --user --upgrade kilm - For pipx: Reinstall with `pipx uninstall...
(QB_NEW_EN)
[grammar] ~94-~94: There might be a mistake here.
Context: ...stallation method:** - Install via pipx for best CLI tool experience: `pipx install...
(QB_NEW_EN)
[grammar] ~96-~96: There might be a mistake here.
Context: ...kilm` Network/PyPI connection issues: - Check internet connection - Try again la...
(QB_NEW_EN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (windows-latest, 3.13)
🔇 Additional comments (16)
pyproject.toml (1)
7-7: Version bump to 0.4.0 — LGTM.Matches the release intent and the package’s exposed version.
docs/astro.config.mjs (1)
63-66: Add “sync” to sidebar — LGTM.Nav reflects the new command; order (sync before update) makes sense.
docs/src/content/docs/reference/cli/index.mdx (1)
21-24: Command roles clarified — LGTM.sync updates libraries; update self-updates KiLM; add-hook points to sync.
kicad_lib_manager/utils/git_utils.py (1)
134-134: Sync wording change in hook content looks goodHeader, echo lines, and command updated to “sync” are consistent with the PR intent.
Also applies to: 137-139, 143-143
kicad_lib_manager/commands/add_hook/docs.mdx (2)
8-8: Good: docs now describe runningkilm syncfrom the hook
161-163: Nice cross-link to sync referencedocs/src/content/docs/guides/automatic-updates.mdx (1)
10-10: Guide correctly pivots tokilm syncAlso applies to: 20-21
kicad_lib_manager/config.py (2)
326-359: LGTM: time-based update check logic is soundReasonable defaults, safe fallbacks, and tolerates unreadable cache.
360-367: LGTM: marking last checkCache path creation and timestamp write are correct.
kicad_lib_manager/cli.py (2)
18-18: Sync command import — LGTMImport wiring looks correct and matches the new package layout.
43-44: Command registration — LGTMRegistering the new sync command on the main group is correct and consistent with tests/docs.
kicad_lib_manager/commands/sync/__init__.py (1)
1-7: Public re-export — LGTMThe narrow surface (all = ["sync"]) matches the new CLI integration.
kicad_lib_manager/commands/update/__init__.py (1)
1-3: No stale imports orkilm updatetest expectations found
Verified that allcheck_for_library_changesreferences now reside undercommands.sync, and no tests still invokekilm updatefor library synchronization.tests/test_sync_command.py (2)
87-96: Core sync test — LGTMCovers the happy path and aligns assertions with new messaging.
12-13: Import path — LGTMImporting check_for_library_changes from the new sync location is correct.
kicad_lib_manager/commands/update/command.py (1)
27-95: Overall update flow LGTMClear messaging, --check/--force semantics, and UpdateManager integration look good.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
kicad_lib_manager/config.py (1)
368-379: Avoidbool(self.get(...))inget_update_preferencesReturn coerced booleans to prevent "false"/"0" becoming True.
return { - "update_check": bool(self.get("update_check", True)), - "update_check_frequency": str(self.get("update_check_frequency", "daily")), - "auto_update": bool(self.get("auto_update", False)), + "update_check": ( + self._coerce_to_bool(self.get("update_check", True)) # type: ignore[arg-type] + if isinstance(self.get("update_check", True), (str, bool)) + else bool(self.get("update_check", True)) + ), + "update_check_frequency": str(self.get("update_check_frequency", "daily")), + "auto_update": ( + self._coerce_to_bool(self.get("auto_update", False)) # type: ignore[arg-type] + if isinstance(self.get("auto_update", False), (str, bool)) + else bool(self.get("auto_update", False)) + ), }kicad_lib_manager/auto_update.py (1)
167-171: Pass running version toPyPIVersionCheckerfor accurate User-AgentAvoid stale UA; improves telemetry/debugging.
- self.version_checker = PyPIVersionChecker("kilm") + self.version_checker = PyPIVersionChecker("kilm", version=self.current_version)kicad_lib_manager/commands/sync/command.py (1)
164-171: Verify setup import path is valid at runtimeEnsure kicad_lib_manager/commands/setup/init.py re-exports setup; otherwise import from .command.
Would you run this to confirm?
#!/bin/bash # Verify setup is exported from package init set -e fd -a 'kicad_lib_manager/commands/setup/__init__.py' || true rg -n 'from \.command import setup|__all__\s*=.*setup|setup\s*=' kicad_lib_manager/commands/setup/__init__.py || echo "setup not exported via __init__" rg -n '^@click\.command\(|def setup\(' kicad_lib_manager/commands/setup/command.py -n -C1
🧹 Nitpick comments (13)
.pre-commit-config.yaml (3)
3-9: Deduplicate Ruff repo entriesYou can declare both hooks under a single repo entry to reduce duplication.
repos: # Ruff formatter + linter - - repo: https://github.com/astral-sh/ruff-pre-commit - rev: v0.1.15 - hooks: - - id: ruff-format - name: ruff format - description: Format Python code with ruff - - # Ruff linter - - repo: https://github.com/astral-sh/ruff-pre-commit + - repo: https://github.com/astral-sh/ruff-pre-commit rev: v0.1.15 hooks: + - id: ruff-format + name: ruff format + description: Format Python code with ruff - id: ruff name: ruff check --fix description: Lint Python code with ruff and auto-fix issues args: [--fix]Also applies to: 11-18
20-29: Reproducibility for local hooksIf pyrefly is pip-installable, prefer
language: python+additional_dependenciesto avoid relying on the system env. If not, keep as-is.- repo: local hooks: - id: pyrefly name: pyrefly check description: Type check Python code with pyrefly - entry: pyrefly - args: [check] - language: system + entry: pyrefly + args: [check] + language: python + additional_dependencies: [pyrefly==<pin-version>] types: [python]
31-40: Run pytest on push (not every commit) to speed up dev loopGate tests to push stage; keep a lightweight lint/format on commit.
- repo: local hooks: - id: pytest name: pytest description: Run tests with pytest entry: pytest - language: system + language: system types: [python] pass_filenames: false always_run: true + stages: [push]kicad_lib_manager/commands/update/docs.mdx (1)
10-16: Add prominent “See also: sync” linkMake the migration path obvious with a link card to the sync command page.
:::caution[Breaking Change in v0.4.0] The `kilm update` command behavior changed in KiLM v0.4.0: - **Now:** Updates KiLM itself (self-update) - **Previously:** Updated library content (now use `kilm sync`) A deprecation banner will guide users during the transition period. ::: + +<CardGrid> + <LinkCard href="/reference/cli/sync/" title="kilm sync" description="Update library content (moved from `kilm update`)." /> +</CardGrid>kicad_lib_manager/config.py (2)
28-36: IncludeboolinConfigValueBooleans are stored in config; add
boolto the union to satisfy typing and intent.-ConfigValue = Union[str, int, List[LibraryDict]] +ConfigValue = Union[str, int, bool, List[LibraryDict]]
414-446: Optional: accept “on/off” as boolean synonymsMinor UX polish for config values.
- if value_lower in {"true", "1", "yes"}: + if value_lower in {"true", "1", "yes", "on"}: return True - elif value_lower in {"false", "0", "no"}: + elif value_lower in {"false", "0", "no", "off"}: return Falsekicad_lib_manager/auto_update.py (1)
18-54: Remove Homebrew detection to avoid misclassificationBrew-installed Python ≠ brew-installed KiLM; default to pip instead. Also align docstring.
- """ - Detect how KiLM was installed to determine appropriate update strategy. - Returns: 'pipx' | 'pip' | 'pip-venv' | 'uv' | 'conda' - """ + """ + Detect how KiLM was installed to determine appropriate update strategy. + Returns: 'pipx' | 'pip' | 'pip-venv' | 'uv' | 'conda' + """ @@ - # Check for homebrew installation (strict path check) - if str(executable_path).startswith(("/opt/homebrew/", "/usr/local/Cellar/")): - return "homebrew" - # Default to system pip return "pip"Optionally remove "homebrew" from
get_update_instructionas unreachable.instructions = { "pipx": "pipx upgrade kilm", "pip": "pip install --upgrade kilm", "pip-venv": "pip install --upgrade kilm", "uv": "uv tool upgrade kilm", - "conda": "Conda package not yet available (planned for future)", - "homebrew": "Homebrew package not yet available (planned for future)", + "conda": "Conda package not yet available (planned for future)", }kicad_lib_manager/commands/sync/command.py (4)
6-10: Make “up to date” detection robust across Git versionsHandle both “Already up to date.” and “Already up-to-date.” and casing variants.
import subprocess +import re from pathlib import Path @@ - is_updated = "Already up to date" not in output + is_updated = not re.search(r"Already up[- ]to date\.?", output, re.IGNORECASE)Also applies to: 105-108
97-103: Add timeouts to subprocess.run to avoid indefinite hangsLong-running pulls/diffs can stall CLI UX.
result = subprocess.run( ["git", "pull"], cwd=lib_path, capture_output=True, text=True, - check=False, + check=False, + timeout=120, ) @@ result = subprocess.run( - ["git", "diff", "--name-status", "HEAD~1", "HEAD"], + ["git", "diff", "--name-status", "HEAD@{1}..HEAD"], cwd=lib_path, capture_output=True, text=True, - check=False, + check=False, + timeout=20, )Also applies to: 205-211
90-93: Don’t inflate “synced” count in dry-run modeDry-run should not increment updated_count.
if dry_run: click.echo(f" Dry run: would execute 'git pull' in {lib_path}") - updated_count += 1 continue
56-57: Nit: singular/pluralConsider “Syncing 1 KiCad GitHub library…” for n=1. Low priority since it impacts tests.
tests/test_sync_command.py (2)
146-148: Docstring wording: “sync” instead of “update”Keep terminology consistent with the new command.
-def test_sync_with_already_up_to_date(mock_config, mock_path_methods): - """Test update when repositories are already up to date.""" +def test_sync_with_already_up_to_date(mock_config, mock_path_methods): + """Test sync when repositories are already up to date.""" @@ -def test_sync_with_verbose(mock_config, mock_subprocess_run, mock_path_methods): - """Test update with verbose option.""" +def test_sync_with_verbose(mock_config, mock_subprocess_run, mock_path_methods): + """Test sync with verbose option.""" @@ -def test_sync_dry_run(mock_config, mock_subprocess_run, mock_path_methods): - """Test update with dry-run option.""" +def test_sync_dry_run(mock_config, mock_subprocess_run, mock_path_methods): + """Test sync with dry-run option."""Also applies to: 165-167, 174-176
68-95: Reduce global monkeypatching of Path to lower side effectsConsider using tmp_path fixtures and real directories/files instead of overriding Path.truediv/glob globally.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
.pre-commit-config.yaml(1 hunks)kicad_lib_manager/auto_update.py(1 hunks)kicad_lib_manager/commands/sync/__init__.py(1 hunks)kicad_lib_manager/commands/sync/command.py(1 hunks)kicad_lib_manager/commands/update/docs.mdx(2 hunks)kicad_lib_manager/config.py(3 hunks)pyproject.toml(2 hunks)tests/test_sync_command.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pyproject.toml
- kicad_lib_manager/commands/sync/init.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-05T22:52:37.835Z
Learnt from: barisgit
PR: barisgit/KiLM#5
File: docs/src/content/docs/reference/cli/index.mdx:21-24
Timestamp: 2025-09-05T22:52:37.835Z
Learning: In the KiLM project, CLI command documentation pages (like /reference/cli/sync/ and /reference/cli/update/) are autogenerated from docs.mdx files embedded within the command modules (e.g., kicad_lib_manager/commands/sync/docs.mdx, kicad_lib_manager/commands/update/docs.mdx). The links in docs/src/content/docs/reference/cli/index.mdx point to these autogenerated paths rather than static files.
Applied to files:
kicad_lib_manager/commands/update/docs.mdx
📚 Learning: 2025-09-05T07:17:34.953Z
Learnt from: barisgit
PR: barisgit/KiLM#3
File: kicad_lib_manager/__init__.py:5-5
Timestamp: 2025-09-05T07:17:34.953Z
Learning: In the KiLM project, different version numbers are intentionally maintained separately: the main Python package version in kicad_lib_manager/__init__.py, the docs site version in docs/package.json, and example versions in documentation files are not meant to be synchronized.
Applied to files:
kicad_lib_manager/commands/update/docs.mdx
🧬 Code graph analysis (4)
tests/test_sync_command.py (4)
kicad_lib_manager/cli.py (1)
main(26-31)kicad_lib_manager/commands/sync/command.py (2)
sync(36-186)check_for_library_changes(190-252)kicad_lib_manager/config.py (1)
get_libraries(141-156)kicad_lib_manager/commands/update/command.py (2)
update(37-176)check_for_library_changes(180-238)
kicad_lib_manager/auto_update.py (1)
kicad_lib_manager/utils/git_utils.py (1)
create_kilm_hook_content(126-144)
kicad_lib_manager/commands/sync/command.py (3)
kicad_lib_manager/config.py (3)
Config(39-446)get_libraries(141-156)get(70-74)kicad_lib_manager/commands/update/command.py (2)
update(37-176)check_for_library_changes(180-238)tests/test_update_command.py (1)
test_check_for_library_changes(185-213)
kicad_lib_manager/commands/update/docs.mdx (1)
kicad_lib_manager/utils/git_utils.py (1)
create_kilm_hook_content(126-144)
🪛 GitHub Actions: CI
kicad_lib_manager/auto_update.py
[error] 201-201: TypeError: 'type' object is not subscriptable in UpdateManager.perform_update annotation (tuple[bool, str]); Python 3.8 does not support built-in generics. Use 'typing.Tuple[bool, str]' or upgrade to Python 3.9+.
🪛 LanguageTool
kicad_lib_manager/commands/update/docs.mdx
[grammar] ~10-~10: There might be a mistake here.
Context: .... :::caution[Breaking Change in v0.4.0] The kilm update command behavior chang...
(QB_NEW_EN)
[grammar] ~11-~11: There might be a mistake here.
Context: ...command behavior changed in KiLM v0.4.0: - Now: Updates KiLM itself (self-update)...
(QB_NEW_EN)
[grammar] ~12-~12: There might be a mistake here.
Context: ...Now:** Updates KiLM itself (self-update) - Previously: Updated library content (n...
(QB_NEW_EN)
[grammar] ~34-~34: There might be a mistake here.
Context: ...ple:_ kilm update --force - --help: Show the help message and exit. ## Beha...
(QB_NEW_EN)
[grammar] ~40-~40: There might be a mistake here.
Context: ...available version using cached requests. 3. Update Execution: For supported method...
(QB_NEW_EN)
[grammar] ~46-~46: There might be a mistake here.
Context: ... Method | Auto-Update | Update Command | |--------|-------------|----------------...
(QB_NEW_EN)
[grammar] ~47-~47: There might be a mistake here.
Context: ...--------|-------------|----------------| | pipx ✅ | Yes | pipx upgrade kilm...
(QB_NEW_EN)
[grammar] ~48-~48: There might be a mistake here.
Context: ...pipx ✅ | Yes | pipx upgrade kilm | | pip ✅ | Yes | `pip install --upgra...
(QB_NEW_EN)
[grammar] ~49-~49: There might be a mistake here.
Context: ...✅ | Yes | pip install --upgrade kilm | | pip (venv) ✅ | Yes | `pip install ...
(QB_NEW_EN)
[grammar] ~50-~50: There might be a mistake here.
Context: ...✅ | Yes | pip install --upgrade kilm | | uv ✅ | Yes | `uv tool upgrade kilm...
(QB_NEW_EN)
[grammar] ~51-~51: There might be a mistake here.
Context: ...uv* ✅ | Yes | uv tool upgrade kilm | | conda | Unavailable | N/A (planned...
(QB_NEW_EN)
[grammar] ~52-~52: There might be a mistake here.
Context: ...Unavailable | N/A (planned for future) | | homebrew | Unavailable | N/A (plan...
(QB_NEW_EN)
[grammar] ~63-~63: There might be a mistake here.
Context: ... kilm update --check **Update KiLM to latest version:** bash kilm update ...
(QB_NEW_EN)
[grammar] ~89-~89: There might be a mistake here.
Context: ... Update fails with permission errors: - For system pip: Try `pip install --user ...
(QB_NEW_EN)
[grammar] ~90-~90: There might be a mistake here.
Context: ...mission errors:** - For system pip: Try pip install --user --upgrade kilm - For pipx: Reinstall with `pipx uninstall...
(QB_NEW_EN)
[grammar] ~94-~94: There might be a mistake here.
Context: ...stallation method:** - Install via pipx for best CLI tool experience: `pipx install...
(QB_NEW_EN)
[grammar] ~96-~96: There might be a mistake here.
Context: ...kilm` Network/PyPI connection issues: - Check internet connection - Try again la...
(QB_NEW_EN)
🔇 Additional comments (5)
kicad_lib_manager/commands/update/docs.mdx (2)
46-53: Good: conda/homebrew marked unavailable with N/ATable accurately reflects current availability. Matches CLI behavior.
39-43: Update project hook text to callkilm sync, notkilm updateAuto-update hooks should update libraries, not self-update. The current hook content in kicad_lib_manager/utils/git_utils.py still runs
kilm update.Apply this change in kicad_lib_manager/utils/git_utils.py:
echo "Running KiCad Library Manager update..." -kilm update +kilm sync @@ -echo "KiCad libraries update complete." +echo "KiCad libraries sync complete."Likely an incorrect or invalid review comment.
kicad_lib_manager/auto_update.py (3)
56-65: LGTM: dynamic User-AgentUA includes current version parameter. Complements the previous suggestion to pass it in.
14-16: Dependencies declared correctly
requests(>=2.32.4) andpackaging(>=25.0) are already listed under[project].dependenciesin pyproject.toml, so no changes are needed.
18-54: Hook content still useskilm update(cross-file mismatch)The standard hook in utils calls
kilm updatewhich now self-updates; it should runkilm syncfor libraries.See proposed diff in the docs.mdx comment; apply in kicad_lib_manager/utils/git_utils.py.
⛔ Skipped due to learnings
Learnt from: barisgit PR: barisgit/KiLM#5 File: docs/src/content/docs/reference/cli/index.mdx:21-24 Timestamp: 2025-09-05T22:52:37.835Z Learning: In the KiLM project, CLI command documentation pages (like /reference/cli/sync/ and /reference/cli/update/) are autogenerated from docs.mdx files embedded within the command modules (e.g., kicad_lib_manager/commands/sync/docs.mdx, kicad_lib_manager/commands/update/docs.mdx). The links in docs/src/content/docs/reference/cli/index.mdx point to these autogenerated paths rather than static files.
| Check if git pull changes indicate new libraries that would require setup. | ||
| Uses git diff to analyze what files were added/changed in the pull. | ||
| Args: | ||
| lib_path: Path to the library directory | ||
| Returns: | ||
| List of change types found ('symbols', 'footprints', 'templates') or empty list if none | ||
| """ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Detect newly added libs reliably (use HEAD@{1} and restrict to A/R/C)
HEAD~1 only covers the last commit; pulls can bring multiple commits. Also, “M” should not trigger setup. Use reflog base and restrict to additions/renames/copies.
def check_for_library_changes(lib_path):
"""
- Check if git pull changes indicate new libraries that would require setup.
- Uses git diff to analyze what files were added/changed in the pull.
+ Check if the pull introduced new libraries that would require setup.
+ Uses git diff to analyze files added/renamed/copied in the pull.
@@
- result = subprocess.run(
- ["git", "diff", "--name-status", "HEAD~1", "HEAD"],
+ result = subprocess.run(
+ ["git", "diff", "--name-status", "HEAD@{1}..HEAD"],
cwd=lib_path,
capture_output=True,
text=True,
check=False,
)
@@
- status, path = parts
- # Only consider added (A) or modified (M) files, ignore deletions
- if status not in ["A", "M", "R"]:
+ status, path = parts
+ # Only consider additions/renames/copies; ignore modifications/deletions
+ if status not in ["A", "R", "C"]:
continueAlso applies to: 205-207, 231-234
🤖 Prompt for AI Agents
In kicad_lib_manager/commands/sync/command.py around lines 192-200 (and
similarly apply to lines 205-207 and 231-234): the git-diff uses HEAD~1 and
inspects all change types including "M", which misses multi-commit pulls and
wrongly treats modifications as new libraries; change the diff base to the
reflog entry HEAD@{1} (the pre-pull state) and restrict the diff to only A,R,C
(added, renamed, copied) via git diff-filter so only newly added/renamed/copied
files trigger setup detection; update all occurrences at the referenced line
ranges accordingly.
| def should_check_updates(self) -> bool: | ||
| """ | ||
| Determine if update check should run based on configuration. | ||
| Returns: | ||
| True if update check should be performed | ||
| """ | ||
| if not self.get("update_check", True): | ||
| return False | ||
|
|
||
| frequency = self.get("update_check_frequency", "daily") | ||
| if frequency == "never": | ||
| return False | ||
|
|
||
| # Check if we've checked recently based on frequency | ||
| cache_dir = Path.home() / ".cache" / "kilm" | ||
| last_check_file = cache_dir / "last_update_check" | ||
|
|
||
| if not last_check_file.exists(): | ||
| return True | ||
|
|
||
| try: | ||
| last_check = float(last_check_file.read_text().strip()) | ||
| now = time.time() | ||
|
|
||
| if frequency == "daily": | ||
| return now - last_check > 86400 # 24 hours | ||
| elif frequency == "weekly": | ||
| return now - last_check > 604800 # 7 days | ||
| except (ValueError, OSError): | ||
| return True | ||
|
|
||
| return False | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix truthiness bug in should_check_updates
String values like "false" are truthy; parse strictly.
def should_check_updates(self) -> bool:
@@
- if not self.get("update_check", True):
+ raw_uc = self.get("update_check", True)
+ try:
+ update_check = (
+ self._coerce_to_bool(raw_uc) if isinstance(raw_uc, (str, bool)) else bool(raw_uc)
+ )
+ except ValueError:
+ update_check = True
+ if not update_check:
return False
@@
- frequency = self.get("update_check_frequency", "daily")
+ frequency = str(self.get("update_check_frequency", "daily"))
if frequency == "never":
return FalseCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In kicad_lib_manager/config.py around lines 326 to 359, the current check uses
if not self.get("update_check", True): which treats string values like "false"
as truthy; update the logic to strictly interpret boolean-like strings and
values (e.g. accept True/False booleans and case-insensitive
"true"/"false","yes"/"no","1"/"0") by normalizing the retrieved value (lowercase
strings) and mapping known falsey strings to False and true-like strings to
True, then use that parsed boolean for the early return; keep the existing
behavior for missing keys (default True) and ensure similar strict parsing is
applied to any other config flags if necessary.
| if args[0] == ["git", "diff", "--name-status", "HEAD~1", "HEAD"]: | ||
| result.stdout = "A\tsymbols/newlib.kicad_sym" | ||
| else: | ||
| # This is a git pull call | ||
| result.stdout = "Updating abcd123..efgh456\nsymbols/newlib.kicad_sym | 120 ++++++++++++\n1 file changed" | ||
|
|
||
| return result |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update expectations for reflog-based diff
Align tests with HEAD@{1}..HEAD usage.
- if args[0] == ["git", "diff", "--name-status", "HEAD~1", "HEAD"]:
+ if args[0] == ["git", "diff", "--name-status", "HEAD@{1}..HEAD"]:
result.stdout = "A\tsymbols/newlib.kicad_sym"
@@
- assert second_call_args[0] == ["git", "diff", "--name-status", "HEAD~1", "HEAD"]
+ assert second_call_args[0] == ["git", "diff", "--name-status", "HEAD@{1}..HEAD"]Also applies to: 118-119
🤖 Prompt for AI Agents
In tests/test_sync_command.py around lines 43-49 (and also update the similar
check at lines 118-119), the test stub expects a diff call using "HEAD~1" but
the code under test now uses a reflog range "HEAD@{1}..HEAD"; update the
conditional that matches args[0] to check for ["git", "diff", "--name-status",
"HEAD@{1}..HEAD"] instead of the current "HEAD~1" string, and make the
corresponding change at lines 118-119 so both places simulate the reflog-based
diff and return the same stdout values as before.
| result.stdout = ( | ||
| "A\tsymbols/newlib.kicad_sym\nM\tfootprints/existing.pretty/file.kicad_mod" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reflect “added-only” policy in change detection test
Ignore “M”; assert on newly added footprint directory.
- result.stdout = (
- "A\tsymbols/newlib.kicad_sym\nM\tfootprints/existing.pretty/file.kicad_mod"
- )
+ result.stdout = (
+ "A\tsymbols/newlib.kicad_sym\nA\tfootprints/new.pretty"
+ )
@@
- mock_run.assert_called_once_with(
- ["git", "diff", "--name-status", "HEAD~1", "HEAD"],
+ mock_run.assert_called_once_with(
+ ["git", "diff", "--name-status", "HEAD@{1}..HEAD"],
cwd=tmp_path,
capture_output=True,
text=True,
check=False,
)Also applies to: 224-231
🤖 Prompt for AI Agents
In tests/test_sync_command.py around lines 211-213 (and similarly for 224-231),
the test expects both added and modified entries but the project policy is
"added-only"; update the expected output assertions to ignore the "M" modified
footprint file and assert only on the newly added symbol and the new footprint
directory (e.g., "A\tsymbols/newlib.kicad_sym" and "A\tfootprints/new.pretty/"),
removing any expectation for "M\tfootprints/existing.pretty/file.kicad_mod" and
adjust related assertions to reflect added-only behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
kicad_lib_manager/auto_update.py (3)
6-10: Tighten installation-method detection and fix docstring mismatch
- Avoid conda/homebrew false positives; broaden pipx env detection; align docstring with return values.
Apply:
@@ -import json -import os +import json +import os +import shutil import subprocess import sys import time @@ - Detect how KiLM was installed to determine appropriate update strategy. - Returns: 'pipx' | 'pip' | 'pip-venv' | 'uv' | 'conda' + Detect how KiLM was installed to determine appropriate update strategy. + Returns: 'pipx' | 'pip' | 'pip-venv' | 'uv' | 'conda' | 'homebrew' @@ - if os.environ.get("PIPX_HOME") and "pipx" in str(executable_path): + if (os.environ.get("PIPX_HOME") or os.environ.get("PIPX_BIN_DIR")) and "pipx" in str(executable_path): return "pipx" @@ - # Check for conda installation - if os.environ.get("CONDA_DEFAULT_ENV") or "conda" in str(executable_path): + # Check for conda installation + if os.environ.get("CONDA_PREFIX") or os.environ.get("CONDA_DEFAULT_ENV"): return "conda" @@ - # Check for homebrew installation (strict path check) - if str(executable_path).startswith(("/opt/homebrew/", "/usr/local/Cellar/")): - return "homebrew" + # Check for Homebrew installation by resolving the kilm executable path + bin_path = shutil.which("kilm") + if bin_path and bin_path.startswith(("/opt/homebrew/Cellar/", "/usr/local/Cellar/")): + return "homebrew"Also applies to: 18-53
59-64: Use running version in PyPI client User-AgentPass current_version so UA isn’t “unknown”.
class UpdateManager: @@ - def __init__(self, current_version: str): - self.version_checker = PyPIVersionChecker("kilm") - self.current_version = current_version + def __init__(self, current_version: str): + self.current_version = current_version + self.version_checker = PyPIVersionChecker("kilm", version=current_version)Also applies to: 167-171
14-15: Verify runtime deps are declared: requests, packagingEnsure both are listed in pyproject/requirements; prevent import-time failures.
#!/bin/bash # Verify 'requests' and 'packaging' are declared as runtime dependencies set -euo pipefail echo "Searching dependency manifests..." rg -n "requests|packaging" -C2 pyproject.toml requirements*.txt setup.cfg setup.py || true echo echo "If using pyproject.toml with PEP 621, ensure under [project].dependencies, e.g.:" echo ' dependencies = ["requests>=2.28", "packaging>=23.0"]'
🧹 Nitpick comments (3)
kicad_lib_manager/auto_update.py (3)
71-80: Be friendlier to PyPI rate limits and be explicit about JSONAdd Accept header and fall back to cache on 429.
- headers = {"User-Agent": self.user_agent} + headers = {"User-Agent": self.user_agent, "Accept": "application/json"} @@ - if response.status_code == 304: # Not Modified + if response.status_code == 304: # Not Modified return cached_data.get("version") if cached_data else None + + if response.status_code == 429: # Too Many Requests + return cached_data.get("version") if cached_data else None
108-112: Specify UTF-8 when reading/writing cacheAvoid locale-dependent encoding issues.
- with Path(self.cache_file).open() as f: + with Path(self.cache_file).open(encoding="utf-8") as f: @@ - with Path(self.cache_file).open("w") as f: + with Path(self.cache_file).open("w", encoding="utf-8") as f:Also applies to: 120-121
185-195: More precise instruction for venv installsUse the exact interpreter to avoid mixing environments.
instructions = { "pipx": "pipx upgrade kilm", "pip": "pip install --upgrade kilm", - "pip-venv": "pip install --upgrade kilm", + "pip-venv": f"{sys.executable} -m pip install --upgrade kilm", "uv": "uv tool upgrade kilm", "conda": "Conda package not yet available (planned for future)", "homebrew": "Homebrew package not yet available (planned for future)", }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.pre-commit-config.yaml(1 hunks)kicad_lib_manager/auto_update.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .pre-commit-config.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
kicad_lib_manager/auto_update.py (3)
kicad_lib_manager/commands/update/command.py (1)
update(37-176)kicad_lib_manager/utils/git_utils.py (1)
create_kilm_hook_content(126-144)tests/test_update_command.py (1)
test_update_command(87-101)
🔇 Additional comments (2)
kicad_lib_manager/auto_update.py (2)
176-184: Good: robust PEP 440 version comparisonUsing packaging.version is correct and future-proof.
12-12: Good: Python 3.8 typing compatibilityImporting Tuple and annotating perform_update -> Tuple[bool, str] fixes 3.8 generics.
Also applies to: 201-205
BREAKING CHANGE!
Summary by CodeRabbit