Skip to content

feat(cli): add data-designer --version#599

Merged
eric-tramel merged 2 commits into
mainfrom
codex/issue-597-cli-version
May 4, 2026
Merged

feat(cli): add data-designer --version#599
eric-tramel merged 2 commits into
mainfrom
codex/issue-597-cli-version

Conversation

@eric-tramel
Copy link
Copy Markdown
Contributor

📋 Summary

Adds a top-level data-designer --version flag so users and support workflows can quickly read the installed data-designer package version. The version path uses importlib.metadata.version("data-designer") and avoids default CLI model setup before exiting.

🔗 Related Issue

Fixes #597

🔄 Changes

  • Added an eager Typer root --version option that prints only the resolved package version.
  • Skipped ensure_cli_default_model_settings() for top-level version requests in the console entrypoint.
  • Added CLI tests for the successful version output, missing package error, and bootstrap skip behavior.

🧪 Testing

  • make test passes (not run; focused CLI checks below)
  • Unit tests added/updated
  • E2E tests added/updated (N/A - no E2E surface changed)
  • uv run --package data-designer pytest packages/data-designer/tests/cli/test_main.py
  • uv run --package data-designer data-designer --version
  • uv run ruff check packages/data-designer/src/data_designer/cli/main.py packages/data-designer/tests/cli/test_main.py

✅ Checklist

  • Follows commit message conventions
  • Commits are signed off (DCO)
  • Architecture docs updated (N/A - no architecture docs impacted)

Signed-off-by: Eric W. Tramel <eric.tramel@gmail.com>
@eric-tramel eric-tramel requested a review from a team as a code owner May 4, 2026 16:02
@eric-tramel eric-tramel self-assigned this May 4, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 4, 2026

Greptile Summary

Adds a --version eager option to the data-designer CLI that resolves the package version via importlib.metadata and exits immediately, with a pre-parse guard in main() that skips ensure_cli_default_model_settings() for the version path. The implementation is idiomatic Typer, the test coverage is thorough (success, missing-package error, and bootstrap-skip scenarios), and no correctness issues were found.

Confidence Score: 5/5

Safe to merge — changes are additive, well-tested, and have no impact on existing command paths.

No P0 or P1 issues found. The version callback, bootstrap skip, and all four new test cases are correct. The _is_version_request pre-parse check is a simple string match that could theoretically match --version inside subcommand args, but the only consequence is skipping an inexpensive bootstrap call before Typer errors anyway — not a real defect.

No files require special attention.

Important Files Changed

Filename Overview
packages/data-designer/src/data_designer/cli/main.py Adds --version eager Typer option with callback, _is_version_request pre-parse guard to skip bootstrap, and the app_callback registration — logic is correct and idiomatic.
packages/data-designer/tests/cli/test_main.py Adds four new test cases covering bootstrap skip, multi-flag detection, successful version output, and missing-package error path; existing test updated to pin sys.argv.

Sequence Diagram

sequenceDiagram
    participant U as User
    participant M as main()
    participant IVR as _is_version_request()
    participant BS as ensure_cli_default_model_settings()
    participant A as app() [Typer]
    participant VC as _version_callback()
    participant IM as importlib.metadata

    U->>M: data-designer --version
    M->>IVR: sys.argv[1:] = ["--version"]
    IVR-->>M: True → skip bootstrap
    M->>A: app()
    A->>VC: eager callback(value=True)
    VC->>IM: version("data-designer")
    alt version found
        IM-->>VC: "0.6.0"
        VC->>U: typer.echo("0.6.0")
        VC-->>A: raise typer.Exit(0)
    else PackageNotFoundError
        IM-->>VC: PackageNotFoundError
        VC->>U: typer.echo(error, err=True)
        VC-->>A: raise typer.Exit(1)
    end

    Note over M,BS: Normal command path
    U->>M: data-designer create config.yaml
    M->>IVR: sys.argv[1:] = ["create", "config.yaml"]
    IVR-->>M: False → run bootstrap
    M->>BS: ensure_cli_default_model_settings()
    BS-->>M: done
    M->>A: app()
    A->>U: execute create command
Loading

Reviews (2): Last reviewed commit: "fix(cli): broaden version bootstrap guar..." | Re-trigger Greptile

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 4, 2026

Greptile Summary

This PR adds a --version flag to the data-designer CLI using Typer's eager callback mechanism (importlib.metadata.version), and skips the ensure_cli_default_model_settings() bootstrap when a version request is detected in sys.argv.

Confidence Score: 4/5

Safe to merge; one minor style suggestion on the _is_version_request guard.

Only a P2 finding: _is_version_request checks args[0] rather than membership, causing the bootstrap to run unnecessarily if --version appears after another flag. No logic errors or correctness issues found.

No files require special attention.

Important Files Changed

Filename Overview
packages/data-designer/src/data_designer/cli/main.py Adds --version eager Typer option, a _version_callback for metadata lookup, and a _is_version_request helper that skips bootstrap in main(). The helper only checks args[0], so --version appearing after other flags still triggers bootstrap.
packages/data-designer/tests/cli/test_main.py Adds three new tests covering bootstrap-skip behaviour, successful version output, and the missing-package error path. All assertions are correct given the default CliRunner mixes stderr into result.output.

Sequence Diagram

sequenceDiagram
    participant User
    participant main as main()
    participant isvr as _is_version_request
    participant bootstrap as ensure_cli_default_model_settings
    participant app as Typer app
    participant vc as _version_callback
    participant meta as importlib.metadata

    User->>main: "data-designer --version"
    main->>isvr: "sys.argv[1:] = ['--version']"
    isvr-->>main: True (skip bootstrap)
    main->>app: app()
    app->>vc: "is_eager=True, value=True"
    vc->>meta: "version('data-designer')"
    meta-->>vc: "0.6.0"
    vc-->>User: echo version string
    vc->>app: raise typer.Exit(0)

    User->>main: "data-designer create config.yaml"
    main->>isvr: "sys.argv[1:] = ['create', ...]"
    isvr-->>main: False
    main->>bootstrap: ensure_cli_default_model_settings()
    main->>app: app()
    app-->>User: run subcommand
Loading
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
packages/data-designer/src/data_designer/cli/main.py:31
**`_is_version_request` only matches `--version` at position 0**

If a user ever passes `--version` after another flag (e.g., `data-designer --config foo.yaml --version`), `args[0]` is not `"--version"`, so `_is_version_request` returns `False` and `ensure_cli_default_model_settings()` runs before Typer's eager callback exits. The version is still printed correctly (Typer's eager handling fires regardless), but the bootstrap runs unnecessarily. Using `"--version" in args` would make the guard consistent with Typer's own behaviour.

```suggestion
    return "--version" in args
```

Reviews (1): Last reviewed commit: "feat(cli): add version flag" | Re-trigger Greptile

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 4, 2026

Thanks for putting this together, @eric-tramel — small, focused PR and a nice match to the intent in #597.

Summary

Adds a top-level data-designer --version flag via a Typer eager callback that prints the installed distribution version from importlib.metadata, and short-circuits ensure_cli_default_model_settings() when the invocation is a version request. Tests cover the happy path, missing-package error, and bootstrap-skip behavior. Implementation matches the PR description and the stated expectations in the linked issue.

Findings

Warnings — Worth addressing

packages/data-designer/src/data_designer/cli/main.py:30-31_is_version_request only matches --version at sys.argv[1]

  • What: _is_version_request returns True only when args[0] == "--version". Any other arrangement (e.g. data-designer --help --version, or a future global flag placed before it) still runs ensure_cli_default_model_settings() before the eager callback fires.
  • Why: Typer's is_eager=True guarantees the version callback triggers first inside app(), so the user still gets correct output — but we'd be paying the bootstrap cost we explicitly set out to avoid (the issue calls this out: "--version should be fast and should not initialize default model config files"). This isn't broken today, but the guard is narrower than the property it's meant to protect.
  • Suggestion: Widen to a membership check, e.g. return "--version" in args. It's still a best-effort heuristic (the callback is the source of truth), but it covers the realistic invocations.
def _is_version_request(args: list[str]) -> bool:
    return "--version" in args

Suggestions — Take it or leave it

packages/data-designer/src/data_designer/cli/main.py:19-27 — lightly duplicated with get_library_version()

  • What: cli/utils/agent_introspection.py:58 already has get_library_version() doing importlib.metadata.version("data-designer"). The new _version_callback reimplements the lookup inline. The issue specifically called out the pattern in agent_introspection.py.
  • Why: Not a critical duplication — importing from agent_introspection.py would defeat the fast-path goal since that module pulls in repositories and the config package. Still, three call sites now (config.version, agent_introspection.get_library_version, and this new callback) each spell out the same 3-line importlib.metadata.version(...) / PackageNotFoundError dance with subtly different fallback semantics.
  • Suggestion: Optional — if you want to consolidate, a tiny data_designer.cli.version helper (one file, no heavy imports) could expose something like get_cli_package_version() reused by both the eager callback and agent_introspection. Happy to leave this for a follow-up; calling it out so the divergence is visible.

packages/data-designer/tests/cli/test_main.py:19-27test_main_bootstraps_before_running_app no longer controls sys.argv

  • What: The existing "bootstrap happens" test relies on the ambient sys.argv left behind by pytest. In practice it's fine (sys.argv[1] under pytest won't be --version), but the test is now implicitly coupled to the runner's invocation.
  • Why: Future changes to _is_version_request or an odd CI invocation (pytest --version ... is the obvious one, though it wouldn't reach this test) could make this flaky. The companion test already shows the pattern.
  • Suggestion: Wrap the body in with patch("sys.argv", ["data-designer"]): to make the bootstrap path explicit and symmetric with the new skip test.

packages/data-designer/src/data_designer/cli/main.py:166_ = version is a bit of a head-scratcher

  • What: Assigning the parameter to _ to silence "unused argument" is unusual for Typer callbacks — the param exists purely so Typer registers the option and fires the eager callback.
  • Why: Minor readability nit. Other Typer patterns in the wild either leave the param truly unused or add a brief comment noting that the callback handles everything.
  • Suggestion: Drop the assignment and add a one-line comment if a linter ever complains: # Parameter is consumed by Typer; _version_callback handles the behavior. Totally optional.

What Looks Good

  • Correct use of is_eager=True — this is the right Typer knob to guarantee --version prints before any option validation or help rendering kicks in.
  • Thoughtful test matrix — happy path, missing-package branch, and the bootstrap-skip behavior are each isolated and assert on observable output (exit code + stdout/stderr), not internal plumbing.
  • Nice touch with raise typer.Exit(1) from None — suppressing the chained traceback keeps the user-facing error clean when the package metadata is genuinely missing.
  • Matches the issue's output contract exactly — plain 0.6.0\n with no decoration, so downstream scripts can parse it without stripping labels.

Verdict

Ship it (with nits) — the _is_version_request narrowness is worth tightening (one-line change), but nothing here blocks merge. The rest are optional polish.


This review was generated by an AI assistant.

Signed-off-by: Eric W. Tramel <eric.tramel@gmail.com>
@eric-tramel
Copy link
Copy Markdown
Contributor Author

Addressed the review feedback in 45b446a1:

  • Broadened _is_version_request() to detect --version anywhere in the root argv list before bootstrapping.
  • Made the bootstrap-path test patch sys.argv explicitly.
  • Added coverage for --version appearing after another root flag.

Re-ran:

  • uv run --package data-designer pytest packages/data-designer/tests/cli/test_main.py
  • uv run --package data-designer data-designer --version
  • uv run ruff check packages/data-designer/src/data_designer/cli/main.py packages/data-designer/tests/cli/test_main.py

@eric-tramel eric-tramel merged commit fc0365c into main May 4, 2026
49 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(cli): add data-designer --version

2 participants