refactor: redesign CLI with positional tool names#197
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #197 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 9 9
Lines 1072 944 -128
==========================================
- Hits 1072 944 -128 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
Walkthrough
ChangesBackend-handler CLI architecture with auto-detect routing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_main.py`:
- Around line 356-375: The test function
test_main_install_auto_detect_tool_name_with_version_fallback can skip the
download path if the tool is already installed in the default directory, making
the test non-deterministic and potentially not exercising the mocked invalid
binary_repo. Add command line arguments to the sys.argv list to specify the
install directory using tmp_path as an isolated location, and change the working
directory to tmp_path (using monkeypatch or a context manager) to ensure the
test always exercises the fallback code path regardless of what is cached on the
system.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d1edaa8a-420c-4ca9-9b50-85e3186875a2
📒 Files selected for processing (2)
clang_tools/main.pytests/test_main.py
shenxianpeng
changed the title
github-actions
Bot
added
the
documentation
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_main.py`:
- Around line 610-632: The test_main_install_backend_binary_download_error test
is not hermetic because it uses the default installation directory which may
already contain the binary, causing the test to skip the download path and
produce non-deterministic results. Add the tmp_path pytest fixture parameter to
the function signature and modify the sys.argv list to include an --install-dir
argument that points to this temporary directory. This ensures a clean, isolated
directory is used for each test run and guarantees the download failure path is
always exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 179743a7-0a94-46c5-aca3-42f71a7ed153
📒 Files selected for processing (2)
clang_tools/main.pytests/test_main.py
Comment on lines
+610
to
+632
| def test_main_install_backend_binary_download_error( | ||
| monkeypatch: pytest.MonkeyPatch, capsys | ||
| ): | ||
| """``--backend binary`` that fails reports the error and returns 1.""" | ||
| monkeypatch.setattr("clang_tools.install.binary_repo", "not-a-valid-url") | ||
| monkeypatch.setattr( | ||
| sys, | ||
| "argv", | ||
| ["clang-tools", "install", "0.0.0"], | ||
| [ | ||
| "clang-tools", | ||
| "install", | ||
| "clang-format", | ||
| "--version", | ||
| "12", | ||
| "--backend", | ||
| "binary", | ||
| "--no-progress-bar", | ||
| ], | ||
| ) | ||
| exit_code = main() | ||
| result = capsys.readouterr() | ||
| assert exit_code == 1 | ||
| assert "not a semantic" in result.err | ||
| assert "Binary install failed" in result.err |
Contributor
There was a problem hiding this comment.
Make the binary download error test hermetic by using an isolated install directory.
This test can skip the download path if clang-format-12 is already installed in the default directory. The binary install might succeed (returning 0) or fail differently during SHA verification, making the test non-deterministic.
Use tmp_path as the install directory to ensure the download failure path is always exercised.
Suggested fix
def test_main_install_backend_binary_download_error(
- monkeypatch: pytest.MonkeyPatch, capsys
+ monkeypatch: pytest.MonkeyPatch, capsys, tmp_path
):
"""``--backend binary`` that fails reports the error and returns 1."""
+ monkeypatch.chdir(tmp_path)
monkeypatch.setattr("clang_tools.install.binary_repo", "not-a-valid-url")
monkeypatch.setattr(
sys,
"argv",
[
"clang-tools",
"install",
"clang-format",
"--version",
"12",
"--backend",
"binary",
+ "--directory",
+ str(tmp_path),
"--no-progress-bar",
],
)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_main.py` around lines 610 - 632, The
test_main_install_backend_binary_download_error test is not hermetic because it
uses the default installation directory which may already contain the binary,
causing the test to skip the download path and produce non-deterministic
results. Add the tmp_path pytest fixture parameter to the function signature and
modify the sys.argv list to include an --install-dir argument that points to
this temporary directory. This ensures a clean, isolated directory is used for
each test run and guarantees the download failure path is always exercised.
|
shenxianpeng
added
minor
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
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.
Description
Redesigns the CLI interface based on the design review of #194. Fixes #194 and #195
Motivation
The old CLI had several design problems (see detailed review):
_is_version_like()to guess whether the positional arg is a version or tool nameclang-tools install 18installed 2 tools (via--tooldefault), butclang-tools install clang-formatinstalled 1install 18 --tool clang-formatandinstall clang-format --version 18did different things--binary/--wheelnaming: ambiguous about what each doesNew Design
Install
Uninstall
# Tools are positional, --version is required clang-tools uninstall clang-format --version 12 clang-tools uninstall clang-format clang-tidy --version 12Key Changes
install <version>install <tool> [<tool> ...] --version <VER>--toolflaguninstall <version> -t <tool>uninstall <tool> --version <VER>_is_version_like()heuristicTests
All 155 tests pass.
Summary by CodeRabbit
New Features
Refactor
--versionas an explicit argument instead of positional parameter.Tests