Migrate from typer to cyclopts #23
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideThis PR overhauls the CLI by migrating from typer to cyclopts, refactors command-line parameter definitions and exit handling, updates the test suite to use direct app invocation with pytest and capsys, and adjusts project metadata and documentation accordingly. Class diagram for CLI migration from typer to cycloptsclassDiagram
class App {
+name: str
+register_install_completion_command()
}
class Parameter {
+alias: str
+negative: str
}
class process_precommit {
+precommit_filename: ResolvedExistingFile
+uv_lock_filename: ResolvedExistingFile
+check: bool
+diff: bool
+color: bool
+quiet: bool
+verbose: bool
+__call__() int
}
App <|-- process_precommit
process_precommit o-- Parameter
class precommit_filename_ResolvedExistingFile
class uv_lock_filename_ResolvedExistingFile
class check_bool
class diff_bool
class color_bool
class quiet_bool
class verbose_bool
process_precommit o-- precommit_filename_ResolvedExistingFile
process_precommit o-- uv_lock_filename_ResolvedExistingFile
process_precommit o-- check_bool
process_precommit o-- diff_bool
process_precommit o-- color_bool
process_precommit o-- quiet_bool
process_precommit o-- verbose_bool
class _print_packages {
+changes: dict[str, bool | tuple[str, str]]
+__call__() None
}
class _print_diff {
+precommit_text: str
+fixed_text: str
+precommit_filename: Path
+color: bool
+__call__() None
}
class _print_summary {
+changes: dict[str, bool | tuple[str, str]]
+dry_mode: bool
+__call__() None
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #23 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 4 4
Lines 146 149 +3
Branches 30 29 -1
=========================================
+ Hits 146 149 +3 ☔ View full report in Codecov by Sentry. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- You removed the old version callback but didn’t wire up a
--versionor-Voption in the new cyclopts App, so make sure to add a version parameter or command to preserve that functionality. - The repeated pytest.raises/capsys pattern in the CLI tests adds a lot of boilerplate—consider extracting a small helper or fixture to collapse the invoke+capture sequence.
- To keep boolean options consistent, explicitly declare negative aliases for flags like
color(e.g.--no-color) using Parameter annotations rather than relying on defaults.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- You removed the old version callback but didn’t wire up a `--version` or `-V` option in the new cyclopts App, so make sure to add a version parameter or command to preserve that functionality.
- The repeated pytest.raises/capsys pattern in the CLI tests adds a lot of boilerplate—consider extracting a small helper or fixture to collapse the invoke+capture sequence.
- To keep boolean options consistent, explicitly declare negative aliases for flags like `color` (e.g. `--no-color`) using Parameter annotations rather than relying on defaults.
## Individual Comments
### Comment 1
<location> `src/sync_with_uv/cli.py:54` </location>
<code_context>
-) -> None:
+ check: Annotated[bool, Parameter(negative="")] = False,
+ diff: Annotated[bool, Parameter(negative="")] = False,
+ color: bool = False,
+ quiet: Annotated[bool, Parameter(alias="-q")] = False,
+ verbose: Annotated[bool, Parameter(alias="-v")] = False,
</code_context>
<issue_to_address>
**suggestion:** No alias for color flag may reduce discoverability.
Consider adding an alias to the `color` flag (e.g., `-c`) for consistency with other flags and improved discoverability.
```suggestion
color: Annotated[bool, Parameter(alias="-c")] = False,
```
</issue_to_address>
### Comment 2
<location> `src/sync_with_uv/cli.py:90-95` </location>
<code_context>
+ except Exception as e: # noqa: BLE001
print("Error:", e, file=sys.stderr)
- raise typer.Exit(123) from e
+ return 123
# report the results / change files
if verbose:
</code_context>
<issue_to_address>
**suggestion:** Returning magic number for error code may reduce maintainability.
Consider replacing `123` with a named constant to improve code clarity and future maintainability.
```suggestion
ERROR_EXIT_CODE = 123
try:
user_repo_mappings, user_version_mappings = load_user_mappings()
fixed_text, changes = process_precommit_text(
precommit_text, uv_data, user_repo_mappings, user_version_mappings
)
except Exception as e: # noqa: BLE001
print("Error:", e, file=sys.stderr)
return ERROR_EXIT_CODE
```
</issue_to_address>
### Comment 3
<location> `tests/test_cli.py:12-16` </location>
<code_context>
- result = runner.invoke(app, ["--version"])
- assert result.exit_code == 0
- assert __version__ in result.stdout
+def test_version(capsys: pytest.CaptureFixture[str]) -> None:
+ with pytest.raises(SystemExit) as exc_info:
+ app("--version")
+ assert exc_info.value.code == 0
+ assert capsys.readouterr().out.strip() == __version__
</code_context>
<issue_to_address>
**suggestion (testing):** Test for --version only checks output, not error conditions or alternate invocation styles.
Add tests for invalid CLI arguments and for running the CLI with no arguments to check error handling. Also, confirm the version output includes the required prefix if applicable.
```suggestion
def test_version(capsys: pytest.CaptureFixture[str]) -> None:
with pytest.raises(SystemExit) as exc_info:
app("--version")
assert exc_info.value.code == 0
output = capsys.readouterr().out.strip()
# Check for required prefix if applicable
assert __version__ in output
assert "version" in output.lower() or "sync-with-uv" in output.lower()
def test_invalid_argument(capsys: pytest.CaptureFixture[str]) -> None:
with pytest.raises(SystemExit) as exc_info:
app("--not-a-real-flag")
assert exc_info.value.code != 0
error_output = capsys.readouterr().out.lower()
assert "error" in error_output or "invalid" in error_output or "usage" in error_output
def test_no_arguments(capsys: pytest.CaptureFixture[str]) -> None:
with pytest.raises(SystemExit) as exc_info:
app()
# Accept either help or error, depending on CLI design
assert exc_info.value.code != 0
output = capsys.readouterr().out.lower()
assert "usage" in output or "error" in output or "help" in output
```
</issue_to_address>
### Comment 4
<location> `tests/test_cli.py:19-28` </location>
<code_context>
+def test_process_precommit_cli_check(
</code_context>
<issue_to_address>
**suggestion (testing):** No test for the scenario where no packages need to be changed.
Add a test where input files are already synchronized, verifying that the CLI returns code 0 and produces the correct output in 'check' mode when no changes are required.
</issue_to_address>
### Comment 5
<location> `tests/test_cli.py:49-41` </location>
<code_context>
+def test_process_precommit_cli_check_q(
</code_context>
<issue_to_address>
**suggestion (testing):** Test for quiet mode does not verify error output for failures.
Add a test case for quiet mode with malformed input to confirm error messages are shown as expected.
Suggested implementation:
```python
def test_process_precommit_cli_check_q(
sample_uv_lock: Path,
sample_precommit_config: Path,
capsys: pytest.CaptureFixture[str],
) -> None:
"""Test the CLI without writing changes."""
def test_process_precommit_cli_check_q_malformed_input(
tmp_path: Path,
capsys: pytest.CaptureFixture[str],
) -> None:
"""Test quiet mode with malformed input to confirm error messages are shown."""
# Create a malformed config file
malformed_config = tmp_path / ".pre-commit-config.yaml"
malformed_config.write_text("not: valid: yaml: [") # Invalid YAML
# Simulate CLI call (replace with actual CLI invocation)
import sys
from uv_precommit.cli import process_precommit_cli_check
# This should raise an error and print to stderr
try:
process_precommit_cli_check(
uv_lock_path=None,
precommit_config_path=malformed_config,
quiet=True,
write=False,
)
except Exception:
pass # We expect an error
captured = capsys.readouterr()
assert "error" in captured.err.lower() or "yaml" in captured.err.lower()
```
- If your CLI invocation differs, adjust the call to `process_precommit_cli_check` accordingly.
- Ensure the CLI prints errors to stderr in quiet mode for malformed input.
- If you use a CLI runner (e.g., pytest's `monkeypatch` or `CliRunner`), adapt the test to use that instead.
</issue_to_address>
### Comment 6
<location> `tests/test_cli.py:151-41` </location>
<code_context>
+def test_process_precommit_cli_with_write(
</code_context>
<issue_to_address>
**suggestion (testing):** No test for file write failures or permission errors.
Add a test that simulates an unwritable output file to ensure the CLI handles write errors and provides a clear error message.
Suggested implementation:
```python
def test_process_precommit_cli_with_write(
sample_uv_lock: Path,
sample_precommit_config: Path,
capsys: pytest.CaptureFixture[str],
) -> None:
"""Test the CLI with writing changes."""
def test_process_precommit_cli_write_permission_error(
sample_uv_lock: Path,
sample_precommit_config: Path,
capsys: pytest.CaptureFixture[str],
monkeypatch,
) -> None:
"""Test the CLI handles file write permission errors gracefully."""
def raise_permission_error(*args, **kwargs):
raise PermissionError("Permission denied")
# Patch Path.write_text to simulate permission error
monkeypatch.setattr("pathlib.Path.write_text", raise_permission_error)
# Assuming process_precommit_cli is the CLI entrypoint function
# and it takes the config path and lock path as arguments
# Replace with actual CLI invocation if different
try:
process_precommit_cli(
config_path=sample_precommit_config,
lock_path=sample_uv_lock,
write=True,
)
except PermissionError:
# If the CLI does not catch the error, the test should fail
pytest.fail("CLI did not handle PermissionError")
captured = capsys.readouterr()
assert "Permission denied" in captured.err or "Permission denied" in captured.out
assert "Could not write to file" in captured.err or "Could not write to file" in captured.out
```
- If your CLI entrypoint is not `process_precommit_cli`, replace it with the correct function or CLI invocation.
- Ensure your CLI prints a clear error message like "Could not write to file" when a `PermissionError` occurs. If not, update the CLI code to catch `PermissionError` and print a user-friendly message.
- If you use a different mechanism for capturing output/errors, adjust the assertions accordingly.
</issue_to_address>
### Comment 7
<location> `tests/test_cli.py:179-41` </location>
<code_context>
-def test_cli_exception_handling(tmp_path: Path) -> None:
+def test_cli_exception_handling(
+ tmp_path: Path, capsys: pytest.CaptureFixture[str]
+) -> None:
</code_context>
<issue_to_address>
**suggestion (testing):** Exception handling test only covers malformed uv.lock, not other error scenarios.
Consider adding tests for scenarios like missing files, invalid YAML in pre-commit config, and unexpected internal errors to improve coverage of exception handling.
Suggested implementation:
```python
def test_cli_exception_handling(
tmp_path: Path, capsys: pytest.CaptureFixture[str]
) -> None:
# Malformed uv.lock scenario (existing test)
# ... existing test code ...
def test_cli_missing_uv_lock(tmp_path: Path, capsys: pytest.CaptureFixture[str]) -> None:
"""Test CLI exception handling when uv.lock file is missing."""
# Setup: create a valid pre-commit config but no uv.lock
sample_precommit_config = tmp_path / ".pre-commit-config.yaml"
sample_precommit_config.write_text("repos: []")
# Run CLI
with pytest.raises(SystemExit):
main(["--config", str(sample_precommit_config), "--lock", str(tmp_path / "uv.lock")])
captured = capsys.readouterr()
assert "uv.lock file not found" in captured.err or "No such file" in captured.err
def test_cli_invalid_yaml_precommit(tmp_path: Path, capsys: pytest.CaptureFixture[str]) -> None:
"""Test CLI exception handling for invalid YAML in pre-commit config."""
sample_precommit_config = tmp_path / ".pre-commit-config.yaml"
sample_precommit_config.write_text("repos: [invalid_yaml")
sample_uv_lock = tmp_path / "uv.lock"
sample_uv_lock.write_text("{}")
with pytest.raises(SystemExit):
main(["--config", str(sample_precommit_config), "--lock", str(sample_uv_lock)])
captured = capsys.readouterr()
assert "Error parsing pre-commit config" in captured.err or "could not parse" in captured.err
def test_cli_unexpected_internal_error(tmp_path: Path, capsys: pytest.CaptureFixture[str], monkeypatch) -> None:
"""Test CLI exception handling for unexpected internal errors."""
sample_precommit_config = tmp_path / ".pre-commit-config.yaml"
sample_precommit_config.write_text("repos: []")
sample_uv_lock = tmp_path / "uv.lock"
sample_uv_lock.write_text("{}")
# Monkeypatch a function to raise an exception
monkeypatch.setattr("cli_module.some_function", lambda *a, **kw: (_ for _ in ()).throw(RuntimeError("Unexpected error")))
with pytest.raises(SystemExit):
main(["--config", str(sample_precommit_config), "--lock", str(sample_uv_lock)])
captured = capsys.readouterr()
assert "Unexpected error" in captured.err
```
- Replace `"cli_module.some_function"` with the actual function in your CLI code that you want to simulate an internal error for.
- Ensure that your CLI prints appropriate error messages for missing files and YAML parsing errors, or adjust the assertions to match your actual error output.
- You may need to import `pytest` and `main` at the top of your test file if not already present.
</issue_to_address>
### Comment 8
<location> `src/sync_with_uv/cli.py:44` </location>
<code_context>
@app.default()
def process_precommit( # noqa: C901, PLR0912, PLR0913
*,
precommit_filename: Annotated[
cyclopts.types.ResolvedExistingFile, Parameter(["-p", "--pre-commit-config"])
] = Path(".pre-commit-config.yaml"),
uv_lock_filename: Annotated[
cyclopts.types.ResolvedExistingFile, Parameter(["-u", "--uv-lock"])
] = Path("uv.lock"),
check: Annotated[bool, Parameter(negative="")] = False,
diff: Annotated[bool, Parameter(negative="")] = False,
color: bool = False,
quiet: Annotated[bool, Parameter(alias="-q")] = False,
verbose: Annotated[bool, Parameter(alias="-v")] = False,
) -> int:
"""Sync pre-commit hook versions with uv.lock.
Updates the 'rev' fields in .pre-commit-config.yaml to match the package
versions found in uv.lock, ensuring consistent versions for development tools.
Parameters
----------
precommit_filename:
Path to .pre-commit-config.yaml file to update
uv_lock_filename
Path to uv.lock file containing package versions
check
Don't write the file back, just return the status.
Return code 0 means nothing would change.
Return code 1 means some package versions would be updated.
Return code 123 means there was an internal error.
diff
Don't write the file back,
just output a diff to indicate what changes would be made.
color
Enable colored diff output. Only applies when --diff is given.
quiet
Stop emitting all non-critical output.
Error messages will still be emitted.
verbose
Show detailed information about all packages,
including those that were not changed.
"""
try:
user_repo_mappings, user_version_mappings = load_user_mappings()
uv_data = load_uv_lock(uv_lock_filename)
precommit_text = precommit_filename.read_text(encoding="utf-8")
fixed_text, changes = process_precommit_text(
precommit_text, uv_data, user_repo_mappings, user_version_mappings
)
except Exception as e: # noqa: BLE001
print("Error:", e, file=sys.stderr)
return 123
# report the results / change files
if verbose:
for package, change in changes.items():
if isinstance(change, tuple):
print(f"{package}: {change[0]} -> {change[1]}", file=sys.stderr)
elif change:
print(f"{package}: unchanged", file=sys.stderr)
else:
print(f"{package}: not managed in uv", file=sys.stderr)
print(file=sys.stderr)
# output a diff to to stdout
if diff:
diff_lines = list(
difflib.unified_diff(
precommit_text.splitlines(keepends=True),
fixed_text.splitlines(keepends=True),
fromfile=str(precommit_filename),
tofile=str(precommit_filename),
)
)
if color:
diff_lines = get_colored_diff(diff_lines)
print("\n".join(diff_lines))
# update the file
if not diff and not check:
precommit_filename.write_text(fixed_text, encoding="utf-8")
# print summary
if verbose or not quiet:
print("All done!", file=sys.stderr)
n_changed = n_unchanged = 0
for change in changes.values():
if isinstance(change, tuple):
n_changed += 1
else:
n_unchanged += 1
would_be = "would be " if (diff or check) else ""
print(
f"{n_changed} package {would_be}changed, "
f"{n_unchanged} packages {would_be}left unchanged.",
file=sys.stderr,
)
# return 1 if check and changed
return int(check and fixed_text != precommit_text)
</code_context>
<issue_to_address>
**issue (code-quality):** Low code quality found in process\_precommit - 23% ([`low-code-quality`](https://docs.sourcery.ai/Reference/Default-Rules/comments/low-code-quality/))
<br/><details><summary>Explanation</summary>The quality score for this function is below the quality threshold of 25%.
This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines.
- Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Adds 6 new test scenarios to improve test coverage: - Check mode when files already synchronized (exit code 0) - Quiet mode still shows error messages with malformed input - File write permission errors - Missing uv.lock file handling - Missing pre-commit config file handling All tests pass with 100% CLI code coverage maintained. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Moves file write operation inside try/except block so permission errors are properly caught and return exit code 123 with error message instead of raising unhandled PermissionError. Also removes test_cli_invalid_yaml_precommit_config as the tool uses regex (not YAML parsing), making expected behavior unclear. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The broad exception handler in process_precommit catches all Exception types and masks unexpected errors—consider narrowing it to only the errors you intend to handle or re-raising others so real bugs aren’t swallowed.
- The CLI tests use the same pytest.raises/SystemExit and capsys pattern in every test—extract a helper or use parametrization to reduce duplication and make new scenarios easier to add.
- You moved all help text into function docstrings; please verify cyclopts picks up those docstrings for --help or add explicit help strings to your Parameter declarations so users still see guidance.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The broad exception handler in process_precommit catches all Exception types and masks unexpected errors—consider narrowing it to only the errors you intend to handle or re-raising others so real bugs aren’t swallowed.
- The CLI tests use the same pytest.raises/SystemExit and capsys pattern in every test—extract a helper or use parametrization to reduce duplication and make new scenarios easier to add.
- You moved all help text into function docstrings; please verify cyclopts picks up those docstrings for --help or add explicit help strings to your Parameter declarations so users still see guidance.
## Individual Comments
### Comment 1
<location> `src/sync_with_uv/cli.py:55` </location>
<code_context>
+ check: Annotated[bool, Parameter(negative="")] = False,
+ diff: Annotated[bool, Parameter(negative="")] = False,
+ color: bool = False,
+ quiet: Annotated[bool, Parameter(alias="-q")] = False,
+ verbose: Annotated[bool, Parameter(alias="-v")] = False,
+) -> int:
</code_context>
<issue_to_address>
**suggestion:** Consider supporting '--quiet' long flag for consistency.
Adding '--quiet' as an alias would align with the convention used for other flags and improve usability.
```suggestion
quiet: Annotated[bool, Parameter(alias=("-q", "--quiet"))] = False,
```
</issue_to_address>
### Comment 2
<location> `CHANGELOG.md:14` </location>
<code_context>
- Support repo URLs from any Git provider (not just GitHub) by using URL parsing instead of regex
- Trigger the hook also on changes of the pyproject.toml file
+- Migrate CLI framework from typer to cyclopts
+- Fix file write permission errors to return exit code 123 instead of raising unhandled exception
## v0.4.0
</code_context>
<issue_to_address>
**suggestion (typo):** Add 'an' before 'unhandled exception' for grammatical correctness.
It should be 'raising an unhandled exception' for correct grammar.
```suggestion
- Fix file write permission errors to return exit code 123 instead of raising an unhandled exception
```
</issue_to_address>
### Comment 3
<location> `src/sync_with_uv/cli.py:44` </location>
<code_context>
[email protected]()
-def process_precommit( # noqa: C901, PLR0912, PLR0913
[email protected]()
+def process_precommit( # noqa: PLR0913
+ *,
precommit_filename: Annotated[
</code_context>
<issue_to_address>
**issue (complexity):** Consider keeping the new helper functions and cyclopts annotations, as they improve clarity and maintain idiomatic usage.
No suggestions—splitting the big function into `_print_*` helpers actually simplifies the main flow, and the cyclopts annotations (while more verbose) are idiomatic for that library.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| check: Annotated[bool, Parameter(negative="")] = False, | ||
| diff: Annotated[bool, Parameter(negative="")] = False, | ||
| color: bool = False, | ||
| quiet: Annotated[bool, Parameter(alias="-q")] = False, |
There was a problem hiding this comment.
suggestion: Consider supporting '--quiet' long flag for consistency.
Adding '--quiet' as an alias would align with the convention used for other flags and improve usability.
Suggested change
| quiet: Annotated[bool, Parameter(alias="-q")] = False, | |
| quiet: Annotated[bool, Parameter(alias=("-q", "--quiet"))] = False, |
| @app.command() | ||
| def process_precommit( # noqa: C901, PLR0912, PLR0913 | ||
| @app.default() | ||
| def process_precommit( # noqa: PLR0913 |
There was a problem hiding this comment.
issue (complexity): Consider keeping the new helper functions and cyclopts annotations, as they improve clarity and maintain idiomatic usage.
No suggestions—splitting the big function into _print_* helpers actually simplifies the main flow, and the cyclopts annotations (while more verbose) are idiomatic for that library.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
aliasparameter instead of shorthand list notationSummary by Sourcery
Migrate the command-line interface from Typer to Cyclopts and adjust related components accordingly.
New Features:
Bug Fixes:
Enhancements:
Build:
Documentation:
Tests:
Chores: