refactor(cli): replace lola mod search with top-level lola search#124
refactor(cli): replace lola mod search with top-level lola search#124SecKatie wants to merge 7 commits into
lola mod search with top-level lola search#124Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a top-level ChangesUnified Search Command and Mod Compatibility
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Pull request overview
Refactors CLI search behavior to reduce confusion: lola mod search now searches the local module registry, while remote marketplace discovery is moved to a new lola market search command (a breaking change for scripts relying on the old behavior).
Changes:
- Repurposes
lola mod searchto do a case-insensitive substring search across local module names, skills, commands, and agents. - Adds
lola market searchto search across enabled marketplace caches (previousmod searchbehavior). - Updates docs and CLI reference to reflect the new command split and breaking change.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lola/cli/mod.py | Changes mod search to search the local registry and formats local results for display. |
| src/lola/cli/market.py | Adds new market search subcommand that delegates to MarketplaceRegistry.search(). |
| docs/guides/marketplace.md | Updates marketplace search example to use lola market search. |
| docs/getting-started/quick-start.md | Updates quick-start marketplace search example to use lola market search. |
| docs/cli-reference/index.md | Updates mod search description and adds market search to the command table. |
| CLAUDE.md | Updates recent-changes notes to reflect the new command split. |
| AGENTS.md | Updates architecture notes to distinguish local vs marketplace search commands. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
SecKatie
changed the title
mod search and market searchlola mod search with top-level lola search
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lola/cli/search.py (1)
60-65: ⚡ Quick winConsider adding validation to prevent confusion when both
--localand--remoteare provided.On lines 60-65, both
--localand--remotemap to the samescopeparameter. When both flags are provided, Click applies "last one wins" (standard Click behavior), solola search git --local --remotesilently searches remote only. While this works correctly, users might not expect this behavior. Adding a validation callback to reject both flags together or documenting the behavior explicitly would improve clarity.🤖 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 `@src/lola/cli/search.py` around lines 60 - 65, The two Click options "--local" and "--remote" both write to the same "scope" parameter, which lets Click silently accept both and use the last one; add a validation callback on the Click options (or on the command) to detect when both flags are passed and raise a Click.UsageError, or explicitly document the "last-one-wins" behavior; specifically modify the Click option declarations for "--local" and "--remote" (or the command function handling search, e.g., the search(...) handler that accepts scope) to include a callback that checks if both flags were set and rejects the invocation with a clear error message like "Cannot specify both --local and --remote".
🤖 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.
Nitpick comments:
In `@src/lola/cli/search.py`:
- Around line 60-65: The two Click options "--local" and "--remote" both write
to the same "scope" parameter, which lets Click silently accept both and use the
last one; add a validation callback on the Click options (or on the command) to
detect when both flags are passed and raise a Click.UsageError, or explicitly
document the "last-one-wins" behavior; specifically modify the Click option
declarations for "--local" and "--remote" (or the command function handling
search, e.g., the search(...) handler that accepts scope) to include a callback
that checks if both flags were set and rejects the invocation with a clear error
message like "Cannot specify both --local and --remote".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f48821f9-cad9-484e-a2a4-9b7139074c0d
📒 Files selected for processing (9)
AGENTS.mdCLAUDE.mddocs/cli-reference/index.mddocs/getting-started/quick-start.mddocs/guides/marketplace.mdsrc/lola/__main__.pysrc/lola/cli/mod.pysrc/lola/cli/search.pytests/test_cli_search.py
💤 Files with no reviewable changes (1)
- src/lola/cli/mod.py
✅ Files skipped from review due to trivial changes (2)
- AGENTS.md
- docs/getting-started/quick-start.md
|
Removing |
|
Finally getting back to this. I'll submit a fix here shortly. |
|
@mrbrandao I resolved the comments and updated the PR! |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_cli_search.py (1)
299-309: ⚡ Quick winAssert stderr-only deprecation behavior explicitly.
This test only verifies that “deprecated” appears somewhere in output, but it doesn’t enforce the compatibility contract that the warning must be on stderr while stdout stays parser-safe.
🤖 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_cli_search.py` around lines 299 - 309, Update the test_mod_search_emits_deprecation_notice to explicitly assert that the deprecation message is emitted on stderr and that stdout remains parser-safe: after invoking mod with cli_runner, assert that "deprecated" (or the exact deprecation text) appears in result.stderr, assert that result.stdout does not contain the deprecation text (and/or equals the expected parser-safe output, e.g., empty or machine-oriented output), and keep references to the same test function and the imported mod from lola.cli.mod when making these assertions.
🤖 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.
Nitpick comments:
In `@tests/test_cli_search.py`:
- Around line 299-309: Update the test_mod_search_emits_deprecation_notice to
explicitly assert that the deprecation message is emitted on stderr and that
stdout remains parser-safe: after invoking mod with cli_runner, assert that
"deprecated" (or the exact deprecation text) appears in result.stderr, assert
that result.stdout does not contain the deprecation text (and/or equals the
expected parser-safe output, e.g., empty or machine-oriented output), and keep
references to the same test function and the imported mod from lola.cli.mod when
making these assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a5088187-aa0f-4cae-a05c-9adbefdb0296
📒 Files selected for processing (7)
AGENTS.mdCLAUDE.mddocs/cli-reference/index.mddocs/guides/marketplace.mdsrc/lola/cli/mod.pysrc/lola/cli/search.pytests/test_cli_search.py
✅ Files skipped from review due to trivial changes (3)
- docs/guides/marketplace.md
- CLAUDE.md
- docs/cli-reference/index.md
🚧 Files skipped from review as they are similar to previous changes (1)
- AGENTS.md
`lola mod search` previously searched remote marketplaces, which was a confusing name given the command lives under `mod`. Move that behavior to `lola market search` (where it belongs) and repurpose `lola mod search` to search the local module registry by name, skill, command, or agent. Breaking change: scripts using `lola mod search` to search marketplaces must switch to `lola market search`. https://claude.ai/code/session_01EkBsZCC4Uo5MCLyay9mmMn
Replace `lola mod search` and `lola market search` with a single top-level `lola search <query>` that searches both the local registry and all enabled marketplaces, with optional `--local` / `--remote` flags to scope. Local matches use module name, skill name, command name, and agent name. Marketplace matches use module name, description, and tags. Results are rendered in two clearly-labelled sections. Breaking change: scripts using `lola mod search` or `lola market search` must switch to `lola search`. https://claude.ai/code/session_01EkBsZCC4Uo5MCLyay9mmMn
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Covers help/registration, local matching (by module/skill/command/agent name, case-insensitive), remote matching (by name and tag), --local / --remote scope flags, the three empty-result tips, and that `lola mod search` is no longer registered. Also drops the redundant ensure_lola_dirs() call in search_cmd; list_registered_modules() already handles it on the local path. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Import _count_str from lola.cli.mod instead of redefining it in search.py; use it for the section headers in _print_local / _print_marketplace instead of recomputing `plural` inline. - Add a `search_env` fixture in test_cli_search.py that creates market_dir/cache_dir and patches MARKET_DIR/CACHE_DIR via monkeypatch, replacing ~6 lines of repeated setup in each test. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Address PR review feedback: - Rename the `lola search` scope flags from `--local`/`--remote` to `--mod`/`--market` for clearer registry-vs-marketplace naming. - Restore `lola mod search` as a thin compatibility alias that forwards to `lola search <query> --mod`, so existing scripts keep working. The deprecation notice is written to stderr so stdout stays byte-identical to `lola search --mod` for scripts parsing the results. - Keep the mutual-exclusivity guard so `--mod --market` errors clearly. - Update docs (CLI reference, marketplace guide), AGENTS.md, CLAUDE.md, and tests to match. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
SecKatie
force-pushed
the
claude/refactor-search-modules-market-tJsXW
branch
from
4e0b8df to
c5bc148
Compare
lola mod searchpreviously searched remote marketplaces, which was aconfusing name given the command lives under
mod. Replace it with asingle top-level
lola searchthat searches both the local moduleregistry and enabled marketplace caches. Use
--localor--remotetoscope to one side.
Breaking change: scripts using
lola mod searchmust switch tolola search(add--remoteto preserve the previous marketplace-onlybehavior).
Examples
https://claude.ai/code/session_01EkBsZCC4Uo5MCLyay9mmMn
Summary
lola searchcommand that searches the local module registry and enabled marketplace caches in one pass, with--local/--remoteflags to scopelola mod search; uselola search(orlola search --remotefor the prior marketplace-only behavior)Summary by CodeRabbit
New Features
lola searchthat queries both the local registry and enabled marketplaces;lola mod searchis a deprecated alias for local-only searches.Changes
--local/--remotewith mutually exclusive--mod/--marketflags to scope searches.lola install <module>now auto-adds from marketplace when missing locally.Documentation
Tests