Conversation
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdded a storage registry and resolver to select storage backends from filesystem paths or URI schemes (exposing Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CLI\n(slayer/cli.py)
participant Resolver as resolve_storage()\n(slayer/storage/base.py)
participant Registry as _STORAGE_REGISTRY
participant Backend as StorageBackend\n(YAML/SQLite/Custom)
User->>CLI: run command with --storage <path|scheme://...>
CLI->>Resolver: resolve_storage(path)
Resolver->>Registry: lookup scheme (if URI)
alt scheme registered
Registry-->>Resolver: factory callable
Resolver->>Backend: factory(path) -> backend instance
else built-in or no scheme
alt path ext is .db/.sqlite/.sqlite3 or scheme==sqlite
Resolver->>Backend: instantiate SQLiteStorage
else
Resolver->>Backend: instantiate YAMLStorage
end
end
Resolver-->>CLI: return StorageBackend
CLI->>Backend: perform requested operation (models/datasources/ingest/query)
Backend-->>CLI: result/status
CLI-->>User: display output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/interfaces/cli.md (1)
115-122:⚠️ Potential issue | 🟡 MinorDocument the new datasource subcommands here too.
slayer/cli.pynow exposescreate,create-inline,delete, andtestunderdatasources(Lines 214-236), but this section still only showslistandshow. That leaves most of the new CLI surface undiscoverable from the interface docs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/interfaces/cli.md` around lines 115 - 122, The docs under the "slayer datasources" section are missing the recently added subcommands; update docs/interfaces/cli.md to document the new datasources subcommands exposed by slayer/cli.py — specifically add brief examples and descriptions for "create", "create-inline", "delete", and "test" (in addition to the existing "list" and "show"), show example CLI invocations (e.g., creating a datasource, inline creation usage, deleting by name, and testing a connection), and note any behavior such as credential masking for "show" and expected output for "test" so users can discover the full CLI surface.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/configuration/storage.md`:
- Around line 80-88: The example is not self-contained because resolve_storage
and RedisStorage are used but not imported; update the snippet to include the
necessary imports (e.g., import resolve_storage from the storage resolver module
and import RedisStorage from the Redis storage implementation) so the example
can be copy-pasted, then call register_storage("redis", lambda path:
RedisStorage(url=f"redis://{path}")) and demonstrate storage =
resolve_storage("redis://localhost:6379/0") as shown (ensure you reference the
symbols register_storage, RedisStorage, and resolve_storage).
In `@slayer/cli.py`:
- Around line 193-229: The CLI currently accepts a raw --password flag
(ds_inline_parser / create-inline) and the help/example shows a literal
password; change this to avoid leaking secrets by (1) adding a --password-stdin
or --prompt-password option that reads the password securely from stdin/getpass
instead of argv, (2) update the help/example text to show using an env
placeholder like ${DB_PASSWORD} (and mention that datasource configs support
${ENV_VAR} resolution) and remove any examples with literal passwords, and (3)
if keeping the --password flag for backward compat, update its help text to
strongly discourage direct use and prefer the new
--password-stdin/--prompt-password or ${ENV_VAR} approach; make these changes
around ds_inline_parser and the top-level usage examples shown for
create-inline.
- Around line 330-331: The include/exclude list creation should ignore empty or
whitespace-only names (and optionally deduplicate) to avoid "" entries from
trailing commas; update the assignments that build include and exclude
(currently using include = [t.strip() for t in args.include.split(",")] if
args.include else None and similar for exclude) to filter out falsy/empty
strings after strip (e.g., only keep t.strip() values that are non-empty) and,
if desired, remove duplicates while preserving order before assigning the
resulting list or None when empty.
In `@slayer/storage/base.py`:
- Around line 42-48: Normalize the scheme when registering so lookups match
resolve_storage's lowercase behavior: in register_storage(scheme, factory) call
scheme = scheme.lower() (and optionally strip surrounding whitespace) before
assigning to _STORAGE_REGISTRY so entries like "Redis" will match "redis://..."
during resolve_storage lookups; update any docstring/examples to show lowercase
registration if needed.
- Around line 72-77: The current logic strips leading slashes from absolute
sqlite URIs and turns "sqlite:///tmp/slayer.db" into "tmp/slayer.db"; update the
sqlite branch so it preserves a leading slash when remainder is absolute: when
scheme == "sqlite" set db_path to remainder unchanged if
remainder.startswith("/") (preserving "/tmp/slayer.db"), otherwise strip any
accidental leading slash for relative forms (use remainder.lstrip("/") only for
the non-absolute case), then return SQLiteStorage(db_path=db_path) — reference
the variables scheme, remainder and the SQLiteStorage return call to locate the
change.
---
Outside diff comments:
In `@docs/interfaces/cli.md`:
- Around line 115-122: The docs under the "slayer datasources" section are
missing the recently added subcommands; update docs/interfaces/cli.md to
document the new datasources subcommands exposed by slayer/cli.py — specifically
add brief examples and descriptions for "create", "create-inline", "delete", and
"test" (in addition to the existing "list" and "show"), show example CLI
invocations (e.g., creating a datasource, inline creation usage, deleting by
name, and testing a connection), and note any behavior such as credential
masking for "show" and expected output for "test" so users can discover the full
CLI surface.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 25057f5f-31fb-4c1a-ba99-e463ad42e6ff
📒 Files selected for processing (4)
docs/configuration/storage.mddocs/interfaces/cli.mdslayer/cli.pyslayer/storage/base.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@slayer/cli.py`:
- Around line 466-469: The datasource connection test uses
ds.get_connection_string() directly, so any ${ENV_VAR} placeholders are not
resolved; update the test to call ds.resolve_env_vars().get_connection_string()
before creating the SQLAlchemy engine (mirror the behavior in
ingest_datasource()/datasource.resolve_env_vars().get_connection_string()),
e.g., obtain a resolved datasource via resolve_env_vars() and use that resolved
connection string when calling sa.create_engine and testing the connection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 78e6f486-c03c-4633-bdae-9fdb2e9421f1
📒 Files selected for processing (3)
docs/configuration/storage.mdslayer/cli.pyslayer/storage/base.py
🚧 Files skipped from review as they are similar to previous changes (1)
- slayer/storage/base.py
AivanF
force-pushed
the
aivan/dev-1233-cli-improve-help-with-descriptions-and-examples
branch
from
62e19dc to
66a758a
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
slayer/cli.py (1)
441-446: Password precedence:--password-stdinsilently overwrites--password.If a user mistakenly provides both
--password secretand--password-stdin, the stdin value silently overwrites the CLI argument. Consider warning if both are provided.🔧 Proposed fix to warn on conflicting options
if args.password_stdin: + if args.password: + print("Warning: --password-stdin overrides --password", file=sys.stderr) import getpass ds_data["password"] = ( getpass.getpass("Password: ") if sys.stdin.isatty() else sys.stdin.readline().rstrip("\n") )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@slayer/cli.py` around lines 441 - 446, If both args.password_stdin and args.password are provided, currently the stdin value silently overwrites the CLI argument stored in ds_data["password"]; update the logic to detect this conflict (check args.password_stdin and args.password) and emit a clear warning (e.g., via logging.warning or printing to stderr) stating that --password-stdin will override --password, then proceed to set ds_data["password"] from stdin as before; reference args.password_stdin, args.password, and ds_data["password"] when implementing the check and warning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@slayer/cli.py`:
- Around line 441-446: If both args.password_stdin and args.password are
provided, currently the stdin value silently overwrites the CLI argument stored
in ds_data["password"]; update the logic to detect this conflict (check
args.password_stdin and args.password) and emit a clear warning (e.g., via
logging.warning or printing to stderr) stating that --password-stdin will
override --password, then proceed to set ds_data["password"] from stdin as
before; reference args.password_stdin, args.password, and ds_data["password"]
when implementing the check and warning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1ac92c00-cb07-468e-8542-3e75b2304b72
📒 Files selected for processing (3)
docs/configuration/storage.mdslayer/cli.pyslayer/storage/base.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation