-
Notifications
You must be signed in to change notification settings - Fork 0
MPT-16666: initial command structure and entrypoint #4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: MPT-16657-add-coderabitt
Are you sure you want to change the base?
MPT-16666: initial command structure and entrypoint #4
Conversation
📝 WalkthroughWalkthroughThe PR adds a Typer-based CLI module with a migrate command, updates project configuration with dependencies and build system setup, removes an obsolete dummy test, and introduces CLI tests using CliRunner. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks✅ Passed checks (2 passed)
Comment |
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
mpt_tool/cli.py (1)
6-8: Consider removing the redundant callback.The
callback()function duplicates the help text already provided in theTyper()initialization on line 3. Since it serves no additional functional purpose, it can be removed to reduce redundancy.🔎 Proposed refactor
-@app.callback() -def callback() -> None: - """MPT CLI - Migration tool for extensions.""" - - @app.command() def migrate() -> None: """Run the migration process."""
📜 Review details
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
mpt_tool/cli.pypyproject.tomltests/dummy_test.pytests/test_cli.py
💤 Files with no reviewable changes (1)
- tests/dummy_test.py
🔇 Additional comments (9)
pyproject.toml (3)
35-42: LGTM: Build system configuration.The hatchling build system configuration is properly set up with appropriate package inclusion patterns.
183-183: LGTM: Mypy test exclusion.Excluding tests from mypy type checking is a reasonable configuration choice that aligns with the per-file-ignores already defined for ruff.
10-12: The constrainttyper>=0.9.0is permissive, but this is standard practice in Python packaging and allows the latest stable version (0.21.0). No security issues were found in the latest releases.tests/test_cli.py (3)
1-9: LGTM: Test setup is clean.The imports and fixture setup are correct. The
CliRunnerfixture provides good test isolation.
12-16: LGTM: Migrate command test is well-structured.The test correctly validates the command's exit code and output. The assertion on the specific string "Hello World!" is appropriate for this initial implementation.
19-24: LGTM: Help text validation is comprehensive.The test correctly verifies that the help output includes both the CLI description and the available commands.
mpt_tool/cli.py (3)
1-3: LGTM: Typer app initialization.The Typer app is correctly configured with help text and
no_args_is_help=True, which provides a good user experience.
11-14: LGTM: Migrate command placeholder.The migrate command is appropriately implemented as a placeholder for this initial PR. The docstring is clear and
typer.echo()is the correct way to output text.
17-19: LGTM: Entry point is correctly implemented.The
main()function correctly serves as the entry point for the CLI application.
| [project.scripts] | ||
| mpt-tool = "mpt_tool.app:main" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Critical: Incorrect entry point path.
The script entry point references mpt_tool.app:main, but based on the test imports (from mpt_tool.cli import app) and the actual CLI module provided (mpt_tool/cli.py), the correct path should be mpt_tool.cli:main. This will cause the mpt-tool command to fail at runtime.
🔎 Proposed fix
[project.scripts]
-mpt-tool = "mpt_tool.app:main"
+mpt-tool = "mpt_tool.cli:main"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [project.scripts] | |
| mpt-tool = "mpt_tool.app:main" | |
| [project.scripts] | |
| mpt-tool = "mpt_tool.cli:main" |
🤖 Prompt for AI Agents
In pyproject.toml around lines 14 to 15, the console script entry incorrectly
points to mpt_tool.app:main whereas the actual CLI module is mpt_tool/cli.py and
tests import from mpt_tool.cli; update the entry to reference mpt_tool.cli:main
so the mpt-tool command resolves the correct module and function at runtime.



Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.