Skip to content

feat: replace version subcommand with --version flag#10

Merged
anneschuth merged 3 commits into
mainfrom
feat/version-flag
Apr 7, 2026
Merged

feat: replace version subcommand with --version flag#10
anneschuth merged 3 commits into
mainfrom
feat/version-flag

Conversation

@anneschuth

Copy link
Copy Markdown
Member

Summary

  • Replaces zad version subcommand with zad --version / zad -V flag
  • Follows standard CLI convention (git, docker, uv all use --version)

Test plan

  • zad --version prints version
  • zad -V prints version
  • All 54 tests pass

Standard CLI convention - users expect `zad --version`, not `zad version`.
claude[bot]
claude Bot previously requested changes Apr 7, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

🔴 Critical — backwards-incompatible removal

zad version is a documented public command. Removing it violates the project's explicit backwards-compatibility policy:

No removing CLI commands, options, or positional arguments
Deprecation before removal: add a deprecation warning for at least 2 minor versions before removing anything

Scripts, CI pipelines, and documentation using zad version will silently break.

Required fix: keep the version subcommand for at least 2 minor versions, but have it print a deprecation notice and delegate to the same output:

@app.command(hidden=True)
def version() -> None:
    """[Deprecated] Use `zad --version` instead."""
    import warnings
    typer.echo("Warning: `zad version` is deprecated, use `zad --version` instead.", err=True)
    print(f"zad-cli {__version__}")

Also restore the version entry in test_backwards_compat.py—removing it from the baseline is exactly what that test exists to prevent.


🟡 Minor — stray blank line in cli.py at line 109: after removing the version command block there are two consecutive blank lines where one is enough.

Restores `zad version` as a deprecated command with a warning,
per the backwards compatibility policy (deprecation before removal).
@claude claude Bot dismissed their stale review April 7, 2026 13:57

Superseded by new review

claude[bot]
claude Bot previously approved these changes Apr 7, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found.

The implementation correctly:

  • Adds --version / -V as an eager flag on the root callback
  • Keeps the version subcommand with deprecated=True and a deprecation warning on stderr — backwards-compatibility policy satisfied
  • Updates _GlobalOptionsGroup._FLAGS so the new flags are hoisted correctly
  • Adds tests for both the new flag (test_version_flag) and the deprecated subcommand (test_version_subcommand_deprecated)
  • Retains "version" in the test_backwards_compat.py baseline

One extra blank line at cli.py line 121 (after the version command block), but not worth blocking on.

Comment thread src/zad_cli/cli.py


def _version_callback(value: bool) -> None:
if value:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Minor: print() bypasses Typer's output handling. Prefer typer.echo(f"zad-cli {__version__}") for consistency with Typer conventions and to respect NO_COLOR/TERM env vars. (The existing deprecated version command also uses print(), so this is at least internally consistent — but worth cleaning both up together.)

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Backwards compatibility is maintained — version subcommand is kept (deprecated) and remains in EXPECTED_COMMANDS in the backwards-compat test. The hoisting of --version/-V in _GlobalOptionsGroup._FLAGS is correct and consistent with how --verbose is handled. is_eager=True + raise typer.Exit() is the correct Typer pattern.

One minor nit left inline (print vs typer.echo), otherwise this is clean.

@anneschuth anneschuth merged commit 002a3ab into main Apr 7, 2026
11 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.

1 participant