feat: support CLI tool to install clang tool wheels#136
feat: support CLI tool to install clang tool wheels#136shenxianpeng wants to merge 3 commits intomainfrom
Conversation
github-actions
bot
added
the
documentation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #136 +/- ##
==========================================
+ Coverage 94.44% 95.00% +0.55%
==========================================
Files 4 4
Lines 108 120 +12
==========================================
+ Hits 102 114 +12
Misses 6 6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
shenxianpeng
force-pushed
the
feature/support-install-clang-wheel
branch
from
4f23a60 to
33aeacc
Compare
|
Warning Rate limit exceeded@shenxianpeng has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 22 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds a CLI entrypoint Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
README.md (1)
76-104: Consider mentioning supported tools explicitly.The documentation clearly demonstrates usage for both
clang-formatandclang-tidy, but it might be helpful to explicitly state which tools are supported (e.g., "Supports:clang-formatandclang-tidy") to set clear expectations for users.cpp_linter_hooks/util.py (1)
98-103: Improve error message formatting.When
--versionis not specified (None), the error message displays "Failed to install clang-format version None", which is awkward. Consider formatting the message to handle the None case more gracefully.Apply this diff:
if path: print(f"{args.tool} installed at: {path}") return 0 else: - print(f"Failed to install {args.tool} version {args.version}") + version_str = f" version {args.version}" if args.version else "" + print(f"Failed to install {args.tool}{version_str}") return 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(2 hunks)cpp_linter_hooks/util.py(2 hunks)pyproject.toml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cpp_linter_hooks/util.py (2)
cpp_linter_hooks/clang_tidy.py (1)
main(32-36)cpp_linter_hooks/clang_format.py (1)
main(64-72)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test (3.12)
- GitHub Check: test (3.14)
- GitHub Check: Run benchmarks
🔇 Additional comments (3)
pyproject.toml (1)
43-43: LGTM!The new script entry point is correctly registered and follows the same pattern as the existing hook entries.
cpp_linter_hooks/util.py (2)
4-4: LGTM!The import is necessary for the new CLI functionality and follows proper import organization.
106-107: LGTM!The module guard correctly implements the standard pattern for CLI entry points and properly propagates the exit code.
CodSpeed Performance ReportMerging #136 will not alter performanceComparing Summary
Benchmarks breakdown
Footnotes
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_util.py (1)
284-320: Consider using Mock objects for better test verification.The lambda mocks have unused
toolandversionparameters, and the tests don't verify that_resolve_installis called with the expected arguments. Usingunittest.mock.Mockwithassert_called_once_withwould be more idiomatic and provide stronger test coverage.Apply this diff to improve the tests:
@pytest.mark.benchmark def test_main_success(monkeypatch): - # Patch _resolve_install to simulate success - monkeypatch.setattr( - "cpp_linter_hooks.util._resolve_install", - lambda tool, version: "/usr/bin/clang-format", - ) + from unittest.mock import Mock + mock_resolve = Mock(return_value="/usr/bin/clang-format") + monkeypatch.setattr("cpp_linter_hooks.util._resolve_install", mock_resolve) monkeypatch.setattr( sys, "argv", ["util.py", "--tool", "clang-format", "--version", "15.0.7"] ) exit_code = util.main() assert exit_code == 0 + mock_resolve.assert_called_once_with("clang-format", "15.0.7") @pytest.mark.benchmark def test_main_failure(monkeypatch): - # Patch _resolve_install to simulate failure - monkeypatch.setattr( - "cpp_linter_hooks.util._resolve_install", lambda tool, version: None - ) + from unittest.mock import Mock + mock_resolve = Mock(return_value=None) + monkeypatch.setattr("cpp_linter_hooks.util._resolve_install", mock_resolve) monkeypatch.setattr( sys, "argv", ["util.py", "--tool", "clang-format", "--version", "99.99.99"] ) exit_code = util.main() assert exit_code == 1 + mock_resolve.assert_called_once_with("clang-format", "99.99.99") @pytest.mark.benchmark def test_main_default_tool(monkeypatch): - # Patch _resolve_install to simulate success for default tool - monkeypatch.setattr( - "cpp_linter_hooks.util._resolve_install", - lambda tool, version: "/usr/bin/clang-format", - ) + from unittest.mock import Mock + mock_resolve = Mock(return_value="/usr/bin/clang-format") + monkeypatch.setattr("cpp_linter_hooks.util._resolve_install", mock_resolve) monkeypatch.setattr(sys, "argv", ["util.py"]) exit_code = util.main() assert exit_code == 0 + mock_resolve.assert_called_once_with("clang-format", None)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pyproject.toml(2 hunks)tests/test_util.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_util.py (1)
cpp_linter_hooks/util.py (1)
main(92-103)
🪛 Ruff (0.14.2)
tests/test_util.py
289-289: Unused lambda argument: tool
(ARG005)
289-289: Unused lambda argument: version
(ARG005)
302-302: Unused lambda argument: tool
(ARG005)
302-302: Unused lambda argument: version
(ARG005)
316-316: Unused lambda argument: tool
(ARG005)
316-316: Unused lambda argument: version
(ARG005)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test (3.10)
- GitHub Check: test (3.13)
- GitHub Check: test (3.14)
- GitHub Check: test (3.12)
- GitHub Check: Run benchmarks
|
1 participant
Summary by CodeRabbit
New Features
Documentation
Tests