fix: add --version flag to CLI#13
Conversation
Signed-off-by: abhijeet nardele <234410808+abhijeetnardele24-hash@users.noreply.github.com>
|
@abhijeetnardele24-hash is attempting to deploy a commit to the Kiyeon Jeon's projects Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Pull request overview
Adds a global --version flag to the parsemux Typer CLI (while keeping the existing version subcommand), and introduces CLI tests to verify both entrypoints output the expected version string.
Changes:
- Add an eager
--versionoption via@app.callback()and centralize version printing in_print_version(). - Reuse
_print_version()in the existingversionsubcommand to keep output consistent. - Add CLI tests covering
parsemux versionandparsemux --version.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/parsemux/cli/main.py |
Adds global --version handling via Typer callback and refactors version printing into a helper. |
tests/test_cli.py |
Adds tests asserting both the version command and --version flag print parsemux {__version__}. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _version_callback(value: bool) -> None: | ||
| """Handle the global --version flag.""" | ||
| if value: | ||
| _print_version() | ||
| raise typer.Exit() |
There was a problem hiding this comment.
The _version_callback is annotated/used like it returns None, but Click/Typer callbacks can replace the option value with the callback’s return. When --version is not provided, this callback currently returns None, which can cause the version parameter in main() (typed as bool) to receive None at runtime. Consider either returning value from the callback (and annotating it accordingly), or changing the option to Optional[bool] = typer.Option(None, ...) (Typer’s common pattern for eager version flags).
| def _version_callback(value: bool) -> None: | |
| """Handle the global --version flag.""" | |
| if value: | |
| _print_version() | |
| raise typer.Exit() | |
| def _version_callback(value: bool) -> bool: | |
| """Handle the global --version flag.""" | |
| if value: | |
| _print_version() | |
| raise typer.Exit() | |
| return value |
| from typer.testing import CliRunner | ||
|
|
||
| from parsemux import __version__ | ||
| from parsemux.cli.main import app |
There was a problem hiding this comment.
This new test file doesn’t follow the existing tests’ module header convention (module docstring + from __future__ import annotations). For consistency with the rest of the suite (e.g., tests/test_api.py:1-4, tests/test_router.py:1-4), consider adding those at the top of this file as well.
kiyeonjeon21
left a comment
There was a problem hiding this comment.
LGTM! Clean implementation.
is_eager=Truecallback pattern is the correct Typer approach- Good refactor extracting
_print_version()to share with the subcommand - Tests cover both
--versionandversionsubcommand
Merging. Thanks for the first contribution @abhijeetnardele24-hash!
Adds a global --version flag using a Typer callback while keeping the existing version subcommand. Also adds CLI tests for both entrypoints.
Closes #3