Feature/collocated docs#3
Conversation
… create sync script
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. WalkthroughDocs site restructured and expanded, including new MDX pages and styles. Introduces a doc sync script that aggregates embedded command docs into the CLI reference. CLI packages refactored to expose public symbols via init and deeper relative imports. Tests updated to new import paths. Version bumped to 0.3.0; Python min set to 3.8. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant PN as pnpm scripts
participant SYNC as sync-embedded-docs.ts
participant FS as File System
participant Docs as docs/src/content/docs/reference/cli
Dev->>PN: pnpm sync-docs
PN->>SYNC: tsx scripts/sync-embedded-docs.ts sync
SYNC->>FS: Scan kicad_lib_manager/commands/* for docs.(md|mdx) and .command
SYNC->>SYNC: Resolve command name (from .command or dir)
alt Missing frontmatter
SYNC->>SYNC: Generate frontmatter (title/description/label)
SYNC->>SYNC: Strip first heading if present
end
SYNC->>FS: Write Docs/<command>.md(x) (preserve extension)
SYNC-->>Dev: Log synced files
Dev->>PN: pnpm sync-docs:watch
PN->>SYNC: watch mode
SYNC->>FS: Watch embedded docs for changes
FS-->>SYNC: Change event
SYNC->>FS: Re-sync changed doc
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (13)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
kicad_lib_manager/commands/pin/command.py (2)
9-14: Fix incorrect import source for list_libraries.list_libraries is defined in utils/file_ops, not library_manager. Current import will fail at runtime unless re-exported.
-from ...library_manager import find_kicad_config, list_libraries +from ...library_manager import find_kicad_config +from ...utils.file_ops import list_librariesIf you prefer a single import point, re-export list_libraries from library_manager and keep callers consistent.
36-40: Make “-s/-f” imply “--selected” and avoid shadowing built-in all().Docs say -s/-f implicitly activate “selected”, but validation currently runs only when all is False. Also, all shadows the built-in. Compute a local pin_all flag instead.
-@click.option( - "--all/--selected", +@click.option( + "--all/--selected", default=True, show_default=True, help="Pin all available libraries or only selected ones", ) ... -def pin(kicad_lib_dir, symbols, footprints, all, dry_run, max_backups, verbose): +def pin(kicad_lib_dir, symbols, footprints, all, dry_run, max_backups, verbose): """Pin libraries in KiCad for quick access""" + # Derive behavior: if any -s/-f are provided, treat as --selected + pin_all = bool(all) and not (symbols or footprints) ... - if all and not symbols and not footprints: + if pin_all: try: symbol_libs, footprint_libs = list_libraries(kicad_lib_dir) symbols = symbol_libs footprints = footprint_libs ... - if not all and (symbols or footprints): + if not pin_all and (symbols or footprints): try: available_symbols, available_footprints = list_libraries(kicad_lib_dir)Also applies to: 58-66, 82-95, 102-104
kicad_lib_manager/commands/setup/command.py (1)
436-457: Back up tables before modifying them, not after.Backups currently occur post-write, defeating the purpose. Back up first (or move this into add_libraries).
- # Create backups only if changes are needed - if changes_needed and not dry_run: - sym_table = kicad_config / "sym-lib-table" - fp_table = kicad_config / "fp-lib-table" - - if sym_table.exists(): - create_backup(sym_table, max_backups) - click.echo("Created backup of symbol library table") - - if fp_table.exists(): - create_backup(fp_table, max_backups) - click.echo("Created backup of footprint library table") + # Back up tables before applying any modifications + if not dry_run: + sym_table = kicad_config / "sym-lib-table" + fp_table = kicad_config / "fp-lib-table" + if sym_table.exists(): + create_backup(sym_table, max_backups) + if fp_table.exists(): + create_backup(fp_table, max_backups) + # (Optional) echo after successful add_libraries call: + if changes_needed and not dry_run: + if (kicad_config / "sym-lib-table").exists(): + click.echo("Created backup of symbol library table") + if (kicad_config / "fp-lib-table").exists(): + click.echo("Created backup of footprint library table")If minimizing unnecessary backups is important, consider moving backup creation into add_libraries so it only triggers when writes occur.
kicad_lib_manager/commands/setup/docs.mdx (1)
25-70: Align docs with implementation for--all-librariesprecedence
- The docs state that
--symbol-lib-dirs/--threed-lib-dirstake precedence over--all-libraries, but the code checks--all-librariesfirst and ignores specific-library flags when it’s set. Update the documentation to show that--all-librariesoverrides any--symbol-lib-dirsor--threed-lib-dirsvalues.kicad_lib_manager/commands/update/command.py (4)
96-105: Add a timeout and handle hung git processesExternal calls should not be unbounded. Add a timeout and surface timeouts distinctly.
- result = subprocess.run( + result = subprocess.run( ["git", "pull"], cwd=lib_path, capture_output=True, text=True, check=False, + timeout=60, ) @@ - except Exception as e: - click.echo(f" Error: {str(e)}") - failed_count += 1 + except subprocess.TimeoutExpired: + click.echo(" Error: git pull timed out (60s).", err=True) + failed_count += 1 + except Exception as e: + click.echo(f" Error: {str(e)}", err=True) + failed_count += 1Also applies to: 136-139
106-118: Counters don’t update in verbose mode; also handle both “Already up-to-date” variantsRight now,
updated_count/up_to_date_countare only incremented in non-verbose runs. Also, Git sometimes prints “Already up-to-date.” Use a single branch and a regex to detect both.if result.returncode == 0: - output = result.stdout.strip() or "Already up to date." - if verbose: - click.echo(f" Success: {output}") - else: - is_updated = "Already up to date" not in output - if is_updated: - click.echo(" Updated") - updated_count += 1 - else: - click.echo(" Up to date") - up_to_date_count += 1 + output = result.stdout.strip() or "Already up to date." + # Treat both "Already up to date" and "Already up-to-date" + is_updated = not re.search(r"Already up[- ]to date\.?", output, re.IGNORECASE) + if verbose: + click.echo(f" Success: {output}") + else: + click.echo(" Updated" if is_updated else " Up to date") + if is_updated: + updated_count += 1 + else: + up_to_date_count += 1
119-132: “New library content” detection will produce false positives; diff the actual changed filesCurrent heuristic checks for presence of any library files on disk (not newly added). This will repeatedly flag “new content” after any pull. Recommend diffing the last pull’s delta.
- # Check git output for new files in key directories - pattern = r"[^\s]+\s+\|\s+\d+ [+]+(?:-+)?\s+(?:symbols|footprints|templates|3d)" - if re.search(pattern, git_output, re.IGNORECASE): - # This is a basic heuristic that indicates changes in library directories - # For more accuracy, we'll check the actual directories - pass - - # Check for specific library file types - symbols_path = lib_path / "symbols" - footprints_path = lib_path / "footprints" - templates_path = lib_path / "templates" - - # Look for symbol libraries (.kicad_sym files) - if symbols_path.exists() and symbols_path.is_dir() and any( - f.name.endswith(".kicad_sym") for f in symbols_path.glob("**/*.kicad_sym") - ): - changes.append("symbols") - - # Look for footprint libraries (.pretty directories) - if footprints_path.exists() and footprints_path.is_dir() and any( - f.is_dir() and f.name.endswith(".pretty") - for f in footprints_path.glob("**/*.pretty") - ): - changes.append("footprints") - - # Look for project templates (directories with metadata.yaml) - if templates_path.exists() and templates_path.is_dir() and any( - (f / "metadata.yaml").exists() - for f in templates_path.glob("*") - if f.is_dir() - ): - changes.append("templates") + # Inspect files changed by the last pull: prefer reflog range; fallback to last commit + changed_files: list[str] = [] + try: + diff = subprocess.run( + ["git", "-C", str(lib_path), "diff", "--name-only", "--diff-filter=AR", "HEAD@{1}..HEAD"], + capture_output=True, + text=True, + check=False, + timeout=10, + ) + if diff.returncode == 0 and diff.stdout.strip(): + changed_files = diff.stdout.splitlines() + else: + # Fallback: names from last commit + show = subprocess.run( + ["git", "-C", str(lib_path), "show", "--name-only", "--pretty=", "HEAD"], + capture_output=True, + text=True, + check=False, + timeout=10, + ) + if show.returncode == 0 and show.stdout.strip(): + changed_files = show.stdout.splitlines() + except Exception: + changed_files = [] + + # Classify changes + for p in (s.strip().lower() for s in changed_files if s.strip()): + if p.startswith("symbols/") and p.endswith(".kicad_sym"): + if "symbols" not in changes: + changes.append("symbols") + elif p.startswith("footprints/") and (p.endswith(".kicad_mod") or p.endswith(".pretty") or "/.pretty/" in p): + if "footprints" not in changes: + changes.append("footprints") + elif p.startswith("templates/") and p.endswith("metadata.yaml"): + if "templates" not in changes: + changes.append("templates")Additionally, consider removing “3d” from detection here since update operates on
library_type="github"(3D “cloud” libs are handled elsewhere).Also applies to: 190-225
153-165: Avoid CLI-to-CLI invocation and accidental process exit
setupcan callsys.exit(1)on fatal paths. Use Click’smain(..., standalone_mode=False)and trapSystemExitsoupdatedoesn’t terminate mid-run.- ctx = click.Context(setup_cmd) - setup_cmd.invoke(ctx) + try: + # Run with defaults; don't let click/sys.exit terminate this command + setup_cmd.main(args=[], standalone_mode=False) + except SystemExit as e: + if e.code: + click.echo( + f"Error: setup exited with code {e.code}. Please run 'kilm setup' manually.", + err=True, + )Also applies to: 159-161
🧹 Nitpick comments (55)
kicad_lib_manager/commands/status/docs.mdx (1)
6-7: Remove unused imports or use the Card components.
LinkCard,Card, andCardGridaren’t used in this page.Apply this diff to drop the unused import:
-import { LinkCard, Card, CardGrid } from "@astrojs/starlight/components";kicad_lib_manager/commands/update/docs.mdx (2)
3-3: Keep formatting consistent for git commands.Elsewhere in the doc you use backticks for
git pull. Mirror that in the description.-description: Update configured Git-based libraries using git pull. +description: Update configured Git-based libraries using `git pull`.
6-7: Remove unused Card imports or add Card UI.The imported components aren’t used.
-import { LinkCard, Card, CardGrid } from "@astrojs/starlight/components";kicad_lib_manager/commands/add_3d/.command (1)
1-1: Add trailing newline for POSIX text-file convention.Some tools expect newline-terminated files.
-add-3d +add-3d +kicad_lib_manager/commands/unpin/docs.mdx (1)
6-7: Remove unused component imports (or add cards).These components aren’t used in this MDX. Drop the import to avoid lint warnings and dead code, or add the Card UI if intended.
-import { LinkCard, Card, CardGrid } from "@astrojs/starlight/components"; +// import { LinkCard, Card, CardGrid } from "@astrojs/starlight/components";kicad_lib_manager/commands/pin/command.py (1)
139-144: Clarify backup messaging.update_pinned_libraries creates the backup internally; this echo can mislead if no write occurred. Consider printing only when the function actually wrote changes (you can have update_pinned_libraries return a second flag or message).
kicad_lib_manager/commands/pin/docs.mdx (1)
6-7: Drop unused component imports (or use them).Same as unpin: either remove import or add Card UI.
-import { LinkCard, Card, CardGrid } from "@astrojs/starlight/components"; +// import { LinkCard, Card, CardGrid } from "@astrojs/starlight/components";kicad_lib_manager/commands/setup/command.py (2)
42-42: Remove duplicate import inside function.create_backup is already imported at module scope.
- from ...utils.backup import create_backup + # use module-scope create_backup
57-66: Regex safety: constrain replacements.The pattern may over-match if URI contains nested braces. Recommend using a non-greedy inner group and anchoring on balanced “${/…}” segments; add a unit test.
- pattern = r'\(uri "\${(\/[^}]+)}\/(.*?)"\)' + pattern = r'\(uri "\${(\/[^}]+)}\/([^"]+)"\)'Would you like a quick PyTest for fix_invalid_uris() covering macOS/Linux-style paths and a Windows-style case?
docs/astro.config.mjs (2)
6-6: Avoid importing unused plugin.Either enable the plugin or remove the import to keep the bundle and dev logs clean.
-import starlightSiteGraph from "starlight-site-graph"; +// import starlightSiteGraph from "starlight-site-graph";
18-18: If keeping the plugin, enable it via config; otherwise keep the import commented.- // plugins: [starlightSiteGraph()], + // plugins: [starlightSiteGraph()],(Optional) If you want the site graph now:
- // plugins: [starlightSiteGraph()], + plugins: [starlightSiteGraph()],kicad_lib_manager/commands/setup/__init__.py (1)
1-3: Prefer lazy re-export to avoid import-time side effects.
Small win for startup time and circular-import resilience.Apply this diff:
-from .command import setup - -__all__ = ["setup"] +from typing import Any + +def __getattr__(name: str) -> Any: + if name == "setup": + from .command import setup as _setup + return _setup + raise AttributeError(name) + +__all__ = ["setup"]kicad_lib_manager/commands/setup/docs.mdx (1)
6-6: Imported Card components are unused.
Either remove the import or add a “Related commands” section using them.Option A (remove import):
-import { LinkCard, Card, CardGrid } from "@astrojs/starlight/components"; +import { LinkCard, CardGrid } from "@astrojs/starlight/components";Option B (keep import; add section near end):
+## Related + +<CardGrid> + <LinkCard title="status" href="/reference/cli/status/" description="Inspect current KiCad setup" /> + <LinkCard title="pin" href="/reference/cli/pin/" description="Pin libraries for quick access" /> + <LinkCard title="unpin" href="/reference/cli/unpin/" description="Unpin libraries" /> + <LinkCard title="update" href="/reference/cli/update/" description="Update libraries and optionally auto-run setup" /> +</CardGrid>kicad_lib_manager/commands/pin/__init__.py (1)
1-3: Optional: lazy re-export to reduce import costs.
Same pattern as suggested forsetup.-from .command import pin - -__all__ = ["pin"] +from typing import Any + +def __getattr__(name: str) -> Any: + if name == "pin": + from .command import pin as _pin + return _pin + raise AttributeError(name) + +__all__ = ["pin"]kicad_lib_manager/commands/list_libraries/docs.mdx (2)
6-6: Remove unusedCardimport.
CardGridandLinkCardare used;Cardisn’t.-import { LinkCard, Card, CardGrid } from "@astrojs/starlight/components"; +import { LinkCard, CardGrid } from "@astrojs/starlight/components";
94-97: Align error example with actual CLI output.
Docs show a specific message that may not match runtime.-# Directory doesn't exist -kilm list --kicad-lib-dir /nonexistent/path -# Error: Directory '/nonexistent/path' does not exist +# Directory doesn't exist +kilm list --kicad-lib-dir /nonexistent/path +# Error: Error listing libraries: [directory not found]kicad_lib_manager/commands/init/docs.mdx (1)
173-182: Sample metadata: dropversionunless it’s actually written.
updated_withis set in code;versionisn’t obviously present.env_var: "KICAD_LIB_MY_LIBRARY" capabilities: symbols: true footprints: true templates: false -version: "1.0.0" updated_with: "kilm"docs/scripts/sync-embedded-docs.ts (2)
141-143: Log the actual filename, not “.mdx” always.- console.log(`Synced ${commandDoc.commandName}.mdx`); + console.log(`Synced ${path.basename(commandDoc.outputPath)}`);
56-87: Guard against duplicate command names overwriting each other.If two dirs resolve to the same sanitized name, the latter overwrites the former silently.
Minimal warning just before return:
const commandDocs: CommandDoc[] = []; @@ - return commandDocs; + // Warn on duplicates + const seen = new Map<string, string>(); + for (const d of commandDocs) { + const prev = seen.get(d.commandName); + if (prev && prev !== d.embeddedPath) { + console.warn(`Duplicate command name "${d.commandName}" from:\n - ${prev}\n - ${d.embeddedPath}`); + } + seen.set(d.commandName, d.embeddedPath); + } + return commandDocs;docs/package.json (2)
6-11: Add a dedicated clean script in root CI to avoid stale outputs.Recommend wiring
pnpm --filter kilm-docs run sync-docs:cleanin CI before build to prevent drift.
25-29: Pinning dev tooling looks fine; consider adding a “typecheck” script.Optional:
"typecheck": "tsx --no-cache --tsconfig tsconfig.json --version >/dev/null && tsc -p . --noEmit"to catch TS issues in the sync script.docs/src/styles/global.css (1)
62-77: Respect reduced motion preferences for the aurora animation.Pause background animation for users with motion sensitivity.
.aurora-bg { @@ animation: auroraShift 22s ease-in-out infinite; } + @media (prefers-reduced-motion: reduce) { + .aurora-bg { animation: none; } + }kicad_lib_manager/commands/list_libraries/.command (1)
1-1: Add trailing newline for POSIX style consistency.Minor style nit to reduce diff churn across editors.
-list +list +kicad_lib_manager/commands/add_hook/.command (1)
1-1: Add trailing newline for consistency.-add-hook +add-hook +kicad_lib_manager/commands/unpin/command.py (2)
28-31: Avoid shadowing built-inall; improve UX and accuracy for counts and backup path.
- Rename the Click parameter for
--alltounpin_allto avoid overshadowing Python’sall().- Report actual numbers unpinned (diff of before/after), not the length of inputs.
- Capture and print the backup path; use explicit UTF-8 for file I/O.
@@ -@click.option( - "--all", - is_flag=True, - help="Unpin all libraries", -) +@click.option( + "--all", + "unpin_all", + is_flag=True, + help="Unpin all libraries", +) @@ -def unpin(symbols, footprints, all, dry_run, max_backups, verbose): +def unpin(symbols, footprints, unpin_all, dry_run, max_backups, verbose): @@ - if all and (symbols or footprints): + if unpin_all and (symbols or footprints): raise click.UsageError("'--all' cannot be used with '--symbols' or '--footprints'") @@ - if all: + if unpin_all: try: - with kicad_common.open() as f: + with kicad_common.open(encoding="utf-8") as f: config = json.load(f) @@ - if not symbols and not footprints and not all: + if not symbols and not footprints and not unpin_all: click.echo("Error: No libraries specified to unpin", err=True) @@ - with kicad_common.open() as f: + with kicad_common.open(encoding="utf-8") as f: config = json.load(f) @@ - changes_needed = False + changes_needed = False + removed_symbol_count = 0 + removed_fp_count = 0 @@ - if "pinned_symbol_libs" in config["session"] and symbols: + if "pinned_symbol_libs" in config["session"] and symbols: current_symbols = config["session"]["pinned_symbol_libs"] new_symbols = [lib for lib in current_symbols if lib not in symbols] if len(new_symbols) != len(current_symbols): changes_needed = True + removed_symbol_count = len(current_symbols) - len(new_symbols) if not dry_run: config["session"]["pinned_symbol_libs"] = new_symbols @@ - if "pinned_fp_libs" in config["session"] and footprints: + if "pinned_fp_libs" in config["session"] and footprints: current_footprints = config["session"]["pinned_fp_libs"] new_footprints = [ lib for lib in current_footprints if lib not in footprints ] if len(new_footprints) != len(current_footprints): changes_needed = True + removed_fp_count = len(current_footprints) - len(new_footprints) if not dry_run: config["session"]["pinned_fp_libs"] = new_footprints @@ - if changes_needed and not dry_run: + if changes_needed and not dry_run: # Create backup before making changes - create_backup(kicad_common, max_backups) + backup_path = create_backup(kicad_common, max_backups) @@ - with kicad_common.open("w") as f: + with kicad_common.open("w", encoding="utf-8") as f: json.dump(config, f, indent=2) @@ - if changes_needed: + if changes_needed: if dry_run: click.echo( - f"Would unpin {len(symbols) if symbols else 0} symbol and {len(footprints) if footprints else 0} footprint libraries in KiCad" + f"Would unpin {removed_symbol_count} symbol and {removed_fp_count} footprint libraries in KiCad" ) else: click.echo( - f"Unpinned {len(symbols) if symbols else 0} symbol and {len(footprints) if footprints else 0} footprint libraries in KiCad" + f"Unpinned {removed_symbol_count} symbol and {removed_fp_count} footprint libraries in KiCad" ) - click.echo("Created backup of kicad_common.json") + if 'backup_path' in locals(): + click.echo(f"Created backup: {backup_path}") click.echo("Restart KiCad for changes to take effect")Also applies to: 49-54, 70-97, 101-109
118-121: Minor: early-return on missing session duplicative.You read the file twice and check “session” in two places. Consider consolidating to a single read/check.
Also applies to: 124-129
kicad_lib_manager/commands/init/command.py (2)
173-201: YAML template write: ensure valid YAML and encoding.You’re already using UTF-8; consider validating the template once in tests to avoid regressions.
I can add a tiny unit test that parses library_descriptions.yaml with PyYAML.
224-237: Persist more context in metadata (optional).Add CLI version to
updated_with(e.g., kilm vX.Y) for traceability.- metadata["capabilities"] = updated_capabilities - write_github_metadata(current_dir, metadata) + metadata["capabilities"] = updated_capabilities + metadata["updated_with"] = f"kilm {click.get_current_context().command.help.splitlines()[0] if __name__ else ''}" + write_github_metadata(current_dir, metadata)Also applies to: 238-244
kicad_lib_manager/commands/config/command.py (2)
30-45: Avoid shadowing built-inlist.Rename the function but keep the CLI name stable.
-@config.command() -def list(library_type, verbose): +@config.command(name="list") +def list_cmd(library_type, verbose):Also applies to: 45-56
344-363: Removing the current default library: clarify behavior.
Configkeepscurrent_library; removing its entry won’t update it. Make the UX explicit.@@ - # Find libraries matching the name and type + # Find libraries matching the name and type matching_libraries = [] for lib in all_libraries: if lib.get("name") == library_name and (library_type == "all" or lib.get("type") == library_type): matching_libraries.append(lib) @@ - # Remove libraries + # Track if we’re removing the current default + removed_current = any(lib.get("path") == current_library for lib in matching_libraries) + + # Remove libraries removed_count = 0 for lib in matching_libraries: lib_type = lib.get("type", "unknown") removed = config.remove_library(library_name, lib_type) if removed: removed_count += 1 @@ - # Check if we removed the current library - current_library_new = config.get_current_library() - if current_library and current_library != current_library_new: - click.echo( - "Note: Default library was changed as the previous default was removed." - ) + # Note if the previous default was removed + if removed_current: + click.echo("Note: The previous default library was removed.") + click.echo('Run "kilm config set-default" to select a new default.')Also applies to: 381-405
kicad_lib_manager/commands/add_hook/command.py (1)
89-96: Specify encoding on write for parity with reads.Small consistency/portability win.
- with post_merge_hook.open("w") as f: + with post_merge_hook.open("w", encoding="utf-8") as f: f.write(new_content)kicad_lib_manager/commands/add_3d/command.py (1)
154-171: Avoid double recursive scans for model files.You scan once to detect presence and again to count. Consider counting once and using that for both checks.
Apply:
- found_models = False - # Do a quick check for model files - for ext in model_extensions: - if list(directory.glob(f"**/*{ext}")): - found_models = True - break + # Count models once + model_count = sum(len(list(directory.glob(f"**/*{ext}"))) for ext in model_extensions) + found_models = model_count > 0 @@ - # Update metadata with actual model count - model_count = 0 - for ext in model_extensions: - model_count += len(list(directory.glob(f"**/*{ext}"))) + # Update metadata with actual model count (already computed)kicad_lib_manager/commands/status/command.py (1)
21-23: Use config constants instead of hardcoding the config path.Reduces drift with Config implementation.
Example:
- config_file = Path.home() / ".config" / "kicad-lib-manager" / "config.yaml" + from ...config import CONFIG_DIR_NAME, CONFIG_FILE_NAME + config_file = Path.home() / ".config" / CONFIG_DIR_NAME / CONFIG_FILE_NAMEkicad_lib_manager/commands/template/command.py (1)
951-957: Avoid shadowing the json module with a parameter.Rename the option’s parameter to avoid confusion.
-@click.option( - "--json", +@click.option( + "--json", is_flag=True, default=False, help="Output in JSON format", show_default=True, ) -def list_templates_cmd(library, verbose, json): +def list_templates_cmd(library, verbose, json: bool): @@ - if json: - import json as json_lib + if json: + import json as json_libAlternatively, keep the CLI flag as --json but name the parameter as_json: click.option("--json", "as_json", is_flag=True, ...).
Also applies to: 1004-1008
tests/test_unpin_command.py (4)
40-48: Patch target looks right; also stub create_backup to avoid FS side effects.Nice redirect to unpin.command. To keep tests pure when changes are written, stub create_backup alongside find_kicad_config and patch Path.open explicitly.
- with patch('kicad_lib_manager.commands.unpin.command.find_kicad_config') as mock_find_config: + with patch('kicad_lib_manager.commands.unpin.command.find_kicad_config') as mock_find_config, \ + patch('kicad_lib_manager.commands.unpin.command.create_backup'): mock_find_config.return_value = Path("/tmp/kicad") @@ - with patch('builtins.open') as mock_open: + with patch('pathlib.Path.open') as mock_open: mock_open.return_value.__enter__.return_value.read.return_value = '{"session": {"pinned_symbol_libs": [], "pinned_fp_libs": []}}'
56-69: Same here: good target; also patch create_backup to avoid unintended writes.Without stubbing, a successful unpin path will call create_backup. Patch it out.
- with patch('kicad_lib_manager.commands.unpin.command.find_kicad_config') as mock_find_config: + with patch('kicad_lib_manager.commands.unpin.command.find_kicad_config') as mock_find_config, \ + patch('kicad_lib_manager.commands.unpin.command.create_backup'): mock_find_config.return_value = Path("/tmp/kicad") @@ - with patch('builtins.open') as mock_open: + with patch('pathlib.Path.open') as mock_open: mock_open.return_value.__enter__.return_value.read.return_value = '{"session": {"pinned_symbol_libs": ["lib1"], "pinned_fp_libs": []}}'
72-85: Ditto for footprints case; stub create_backup and prefer Path.open.Prevents backup writes and keeps IO mocked consistently.
- with patch('kicad_lib_manager.commands.unpin.command.find_kicad_config') as mock_find_config: + with patch('kicad_lib_manager.commands.unpin.command.find_kicad_config') as mock_find_config, \ + patch('kicad_lib_manager.commands.unpin.command.create_backup'): mock_find_config.return_value = Path("/tmp/kicad") @@ - with patch('builtins.open') as mock_open: + with patch('pathlib.Path.open') as mock_open: mock_open.return_value.__enter__.return_value.read.return_value = '{"session": {"pinned_symbol_libs": [], "pinned_fp_libs": ["lib1"]}}'
40-40: Optionally assert output for stronger guarantees.Instead of exit_code in [0, 1], assert specific messages (e.g., “No pinned libraries found” vs “Unpinned …”). Improves signal.
tests/test_update_command.py (1)
168-173: Good; also simplify auto-setup mocking by patching setup_cmd directly.Avoids sys.modules shimming and couples to the symbol update.command actually uses.
- with patch.dict( - "sys.modules", {"kicad_lib_manager.commands.setup": setup_module_mock} - ): - runner = CliRunner() - result = runner.invoke(main, ["update", "--auto-setup"]) + with patch("kicad_lib_manager.commands.update.command.setup_cmd") as setup_cmd: + runner = CliRunner() + result = runner.invoke(main, ["update", "--auto-setup"]) + setup_cmd.assert_called()kicad_lib_manager/commands/list_libraries/command.py (1)
47-49: Optional: raise ClickException instead of sys.exit for better UX.Leverages Click’s error handling.
- except Exception as e: - click.echo(f"Error listing libraries: {e}", err=True) - sys.exit(1) + except Exception as e: + raise click.ClickException(f"Error listing libraries: {e}")kicad_lib_manager/commands/update/command.py (3)
133-135: Send failures to stderrSurface failures on stderr for better UX and scripting.
- click.echo(f" Failed: {result.stderr.strip()}") + click.echo(f" Failed: {result.stderr.strip()}", err=True) @@ - click.echo(f" Error: {str(e)}") + click.echo(f" Error: {str(e)}", err=True)Also applies to: 137-138
91-95: Dry-run should not increment “updated” countCurrently dry-run adds to
updated_count(“Actually changed”), which skews the summary. Track a separateplanned_update_countor only print the intent without touching counters.
178-178: Move helper to a services/utils moduleAgree with the TODO. Extract
check_for_library_changesinto a testable utility (e.g.,utils/git_changes.py) and add unit tests around the diff classification.docs/README.md (2)
16-26: Surface doc-sync commands and clarify where to run themThese commands execute from the docs/ workspace, and it’s useful to expose the new sync scripts.
Apply:
- All commands are run from the root of the project, from a terminal: + Run these from the docs/ directory: | Command | Action | | :------------- | :------------------------------------------- | | `pnpm install` | Installs dependencies | | `pnpm dev` | Starts local dev server at `localhost:4321` | | `pnpm build` | Build your production site to `./dist/` | | `pnpm preview` | Preview your build locally, before deploying | +| `pnpm sync-docs` | One-shot sync of embedded CLI docs | +| `pnpm sync-docs:watch` | Watch and continuously sync embedded CLI docs | +| `pnpm sync-docs:clean` | Remove generated CLI docs |
29-43: Add a language to the fenced code block (markdownlint MD040)Specifying a language improves rendering and satisfies linting.
Apply:
-``` +```text docs/ ├── public/ # Static assets ├── src/ │ ├── assets/ # Images and other assets │ ├── content/ │ │ ├── docs/ # Documentation content │ │ │ ├── guides/ # User guides and tutorials │ │ │ ├── reference/ # Command reference and API docs │ │ │ └── community/ # Contributing guidelines │ └── content.config.ts # Content collection config ├── astro.config.mjs # Astro configuration ├── package.json └── README.md # This file -``` +```docs/src/content/docs/guides/getting-started.mdx (1)
134-139: Include --env-var in the Consumer 3D registration example for consistencyThe preceding text instructs using the same --env-var as the Creator, but the example omits it.
Apply:
# Example: Pointing to your local path for the shared models kilm add-3d --name shared-3d-models \ - --directory /Users/myuser/GoogleDrive/kicad-3d-models + --directory /Users/myuser/GoogleDrive/kicad-3d-models \ + --env-var ACME_3D_MODELSdocs/src/content/docs/guides/installation.mdx (1)
37-47: Preferuv tool installfor CLI installation
uv pip installtargets the current environment;uv tool installis the intended CLI workflow with shims.Apply:
-# Install KiLM using uv -uv pip install kilm +# Install KiLM as a CLI tool using uv +uv tool install kilmkicad_lib_manager/commands/add_3d/docs.mdx (3)
6-6: Remove unused import.
Cardisn’t used.-import { LinkCard, Card, CardGrid } from "@astrojs/starlight/components"; +import { LinkCard, CardGrid } from "@astrojs/starlight/components";
32-36: Fix grammar and overstatement about metadata contents.
- “associate” → “associated”.
- “Last updated timestamp” isn’t guaranteed by the current implementation; it sets
updated_withandmodel_count, not a timestamp.-1. **Creates/Updates Metadata**: Creates or modifies `.kilm_cloud_metadata` in the target directory containing: - - Library name, description, and associated environment variable - - Count of detected model files - - Last updated timestamp +1. **Creates/Updates Metadata**: Creates or modifies `.kilm_cloud_metadata` in the target directory containing: + - Library name, description, and associated environment variable + - Count of detected model files + - Update markers (e.g., tool/version info)If you do want a timestamp, I can add
metadata["updated_at"] = datetime.utcnow().isoformat()in the command. Want a patch?
32-32: Unify metadata filename across code and docs.This page uses
.kilm_cloud_metadata, while the add_3d docstring references.kilm_metadata. Pick one and make CLI help/messages consistent to avoid confusion.kicad_lib_manager/commands/add_hook/docs.mdx (2)
6-6: Remove unused import.
Cardisn’t used.-import { LinkCard, Card, CardGrid } from "@astrojs/starlight/components"; +import { LinkCard, CardGrid } from "@astrojs/starlight/components";
178-183: Windows note (optional).Git for Windows runs hooks via sh; chmod semantics differ. Consider a brief note that hooks still work on Windows with Git Bash.
docs/src/content/docs/community/development.mdx (2)
12-15: Re-evaluate Python version floor.Python 3.7/3.8 are EOL. Recommend 3.9+ (or 3.10+) unless you must support older.
-- **Python 3.7+**: Required for KiLM development +- **Python 3.9+**: Recommended for KiLM development
125-130: Avoid advertising a non-existent “make quality”.Either add the target or mark as “coming soon”.
-# Run complete quality check (to be implemented) -# This should include: pyrefly, black, ruff, pytest -make quality # or equivalent script +# Run complete quality check (coming soon) +# Intended to include: type-check, black, ruff, pytest +# make quality # or equivalent scriptdocs/src/content/docs/guides/configuration.mdx (1)
64-76: Clarify metadata formats:kilm.yamlvs.kilm_cloud_metadata.Elsewhere (add-3d) we document
.kilm_cloud_metadatafor 3D (cloud) libs. Here we introducekilm.yaml. Explain coexistence and when each is used to avoid confusion.-Each library directory can contain a `kilm.yaml` file with metadata: +Each library directory can contain a `kilm.yaml` file with metadata. +Cloud-based 3D libraries additionally use a per-folder `.kilm_cloud_metadata` file managed by `kilm add-3d`.
1 participant
Make docs be with code so it is easier to maintain.
Summary by CodeRabbit
New Features
Documentation
Chores