refactor(wren): unify datasource resolution from connection_info#1506
refactor(wren): unify datasource resolution from connection_info#1506douenergy merged 2 commits intoCanner:mainfrom
Conversation
Remove --datasource flag from query/dry-run/validate commands — datasource
is now always read from connection_info.json. Only dry-plan retains
--datasource/-d for transpile-only use without a connection file.
Also add _normalize_conn to auto-flatten the MCP/web envelope format
({"datasource": ..., "properties": {...}}) so both flat and nested
connection files work seamlessly.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
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 (2)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughCLI datasource handling was refactored: the Changes
Sequence Diagram(s)(Skipped — changes are documentation and CLI parameter refactors without a new multi-component sequential runtime flow needing visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
wren/src/wren/cli.py (2)
287-309:dry-planlacks--connection-infooption unlike other commands.The
dry-plancommand accepts--connection-filebut not--connection-info(inline JSON). While this is likely intentional sincedry-plandoesn't need full connection credentials, users who are accustomed to passing inline JSON for other commands may be confused.Consider either adding
--connection-infofor consistency (ignoring all fields exceptdatasource), or documenting this difference explicitly in the help text.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren/src/wren/cli.py` around lines 287 - 309, The dry_plan CLI command (function dry_plan) is missing the --connection-info / ConnInfoOpt option for consistency with other commands; add an optional parameter matching the existing ConnInfoOpt type (or reuse the same CLI annotation used elsewhere) to the dry_plan signature alongside connection_file, accept inline JSON but only use its datasource field when resolving datasource (i.e., pass the parsed connection_info into _load_conn/_resolve_datasource or ignore credentials), and update the help string to note that only datasource is considered so behavior matches other commands.
50-60: Envelope flattening may discard extra top-level keys.If the envelope format contains additional top-level keys besides
datasourceandproperties(e.g., metadata fields added by MCP server), they will be silently dropped. If this is intentional, consider adding a brief comment. If not, you may want to merge them:♻️ Optional: preserve additional top-level keys
def _normalize_conn(conn: dict) -> dict: """Flatten the ``{"datasource": ..., "properties": {...}}`` envelope. MCP / web connection files wrap connection fields under a ``properties`` key. This normalises both formats into ``{"datasource": ..., **fields}``. """ if "properties" in conn and isinstance(conn["properties"], dict): props = conn["properties"] props["datasource"] = conn.get("datasource", props.get("datasource")) + # Preserve any additional top-level keys (e.g., metadata) + for key, value in conn.items(): + if key not in ("datasource", "properties") and key not in props: + props[key] = value return props return conn🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren/src/wren/cli.py` around lines 50 - 60, The _normalize_conn function currently returns only props when "properties" exists, dropping any other top-level keys; update it to merge top-level keys with the flattened properties instead of discarding them: create a new dict that starts from conn (excluding the original "properties" key) and then update/override it with props, ensure "datasource" is taken from conn if present else from props, and return this merged result; modify the logic inside _normalize_conn to build and return that merged dict while removing "properties".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@wren/src/wren/cli.py`:
- Around line 287-309: The dry_plan CLI command (function dry_plan) is missing
the --connection-info / ConnInfoOpt option for consistency with other commands;
add an optional parameter matching the existing ConnInfoOpt type (or reuse the
same CLI annotation used elsewhere) to the dry_plan signature alongside
connection_file, accept inline JSON but only use its datasource field when
resolving datasource (i.e., pass the parsed connection_info into
_load_conn/_resolve_datasource or ignore credentials), and update the help
string to note that only datasource is considered so behavior matches other
commands.
- Around line 50-60: The _normalize_conn function currently returns only props
when "properties" exists, dropping any other top-level keys; update it to merge
top-level keys with the flattened properties instead of discarding them: create
a new dict that starts from conn (excluding the original "properties" key) and
then update/override it with props, ensure "datasource" is taken from conn if
present else from props, and return this merged result; modify the logic inside
_normalize_conn to build and return that merged dict while removing
"properties".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 75cbf458-5e3e-4cc4-9ee0-78d67de90e86
📒 Files selected for processing (5)
cli-skills/wren-usage/SKILL.mdcli-skills/wren-usage/references/wren-sql.mdwren/docs/cli.mdwren/docs/connections.mdwren/src/wren/cli.py
Add wren/ as fifth main module in project-level docs, document wren SDK build commands, cli-skills/, and PyPI publishing. Update wren package docs with memory module, expanded connector details, updated build commands, and test structure. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
--datasourceflag fromquery,dry-run,validateCLI commands — datasource is now always read fromconnection_info.json--datasource/-donly ondry-planfor transpile-only use without a connection file_normalize_connto auto-flatten the MCP/web envelope format ({"datasource": ..., "properties": {...}}) so both flat and nested connection files workcli.md,connections.md) and CLI skill (SKILL.md,wren-sql.md)Test plan
wren --sql 'select customer_id from customers' -l 3works with envelope-formatconnection_info.jsonwren dry-plan --sql '...'works with auto-discovered datasource from connection filewren dry-plan --sql '...' -d duckdbworks with explicit datasource override🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
New Features