New CLI to install clang-format and clang-tidy Python wheels #133
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 #133 +/- ##
==========================================
+ Coverage 95.42% 95.77% +0.35%
==========================================
Files 7 9 +2
Lines 284 308 +24
==========================================
+ Hits 271 295 +24
Misses 13 13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
shenxianpeng
force-pushed
the
support-wheel-install
branch
from
2ff41eb to
a21141d
Compare
|
Warning Rate limit exceeded@github-actions[bot] has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 38 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 new CLI and console entrypoint Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related issues
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 (6)
.github/workflows/test.yml (1)
160-182: Version verification logic may be fragile to output format variations.The grep pattern
"version ${{ matrix.version }}"assumes a consistent output format across clang versions and tools. While this likely works for versions >= 13, output formats could vary by:
- Version format: e.g., "version 21" vs. "version 21.0.0" vs. pre-release tags
- Tool variations: clang-tidy may have multi-line output; first line is typically checked, but this depends on the exact format
Consider adding more defensive checks (e.g., exact version extraction and comparison) or documenting the assumed output format if this becomes a maintenance burden.
pyproject.toml (1)
34-36: Consider upper bound on cpp-linter-hooks dependency.The dependency
cpp-linter-hooks>=1.1.6has no upper bound, which could introduce breaking changes if a major version bump occurs. Consider:
- Using a version range like
cpp-linter-hooks>=1.1.6,<2for stability, or- Reviewing cpp-linter-hooks' versioning strategy to confirm it follows semver
Based on learnings, this project is at version 0.x and is tightly coupled to cpp-linter-action, so breaking changes may be acceptable. However, document this assumption.
docs/conf.py (1)
211-250: Excellent refactoring to eliminate code duplication.The
write_cli_dochelper function successfully extracts common documentation generation logic, making it reusable for both CLI tools.Consider whether
REQUIRED_VERSIONS(lines 196-202) should be updated to include version information for the newclang-tools-wheelCLI options. Currently, the wheel CLI options will all default to version "0.1.0" (line 237) since they're not present in the mapping. If the wheel CLI was introduced in a different version, the badges should reflect that.tests/test_wheel.py (3)
5-15: Consider using the correct program name in sys.argv and verifying output.The test uses
"util.py"asargv[0], which doesn't match the actual CLI tool nameclang-tools-wheel. While this doesn't affect the test's current functionality, using the correct program name would make the tests more realistic.Additionally, consider using
capsysto verify the success message is printed correctly, not just the exit code.def test_main_success(monkeypatch): # Patch _resolve_install to simulate success monkeypatch.setattr( "clang_tools.wheel._resolve_install", lambda tool, version: "/usr/bin/clang-format", ) monkeypatch.setattr( - sys, "argv", ["util.py", "--tool", "clang-format", "--version", "15.0.7"] + sys, "argv", ["clang-tools-wheel", "--tool", "clang-format", "--version", "15.0.7"] ) exit_code = main() assert exit_code == 0To verify output, you could add:
def test_main_success(monkeypatch, capsys): # ... existing setup ... exit_code = main() captured = capsys.readouterr() assert exit_code == 0 assert "clang-format installed at: /usr/bin/clang-format" in captured.out
18-27: Consider using the correct program name in sys.argv.Similar to
test_main_success, this test uses"util.py"asargv[0]instead of"clang-tools-wheel".def test_main_failure(monkeypatch): # Patch _resolve_install to simulate failure monkeypatch.setattr( "clang_tools.wheel._resolve_install", lambda tool, version: None ) monkeypatch.setattr( - sys, "argv", ["util.py", "--tool", "clang-format", "--version", "99.99.99"] + sys, "argv", ["clang-tools-wheel", "--tool", "clang-format", "--version", "99.99.99"] ) exit_code = main() assert exit_code == 1
30-38: Consider using the correct program name in sys.argv.Similar to the other tests, this uses
"util.py"asargv[0]instead of"clang-tools-wheel".def test_main_default_tool(monkeypatch): # Patch _resolve_install to simulate success for default tool monkeypatch.setattr( "clang_tools.wheel._resolve_install", lambda tool, version: "/usr/bin/clang-format", ) - monkeypatch.setattr(sys, "argv", ["util.py"]) + monkeypatch.setattr(sys, "argv", ["clang-tools-wheel"]) exit_code = main() assert exit_code == 0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.github/workflows/test.yml(5 hunks).pre-commit-config.yaml(2 hunks)README.rst(5 hunks)clang_tools/wheel.py(1 hunks)docs/api.rst(1 hunks)docs/conf.py(2 hunks)docs/index.rst(1 hunks)docs/usage.rst(1 hunks)docs/wheel_cli_args.rst(1 hunks)pyproject.toml(1 hunks)tests/test_wheel.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-13T10:24:01.545Z
Learnt from: 2bndy5
Repo: cpp-linter/clang-tools-pip PR: 122
File: pyproject.toml:95-98
Timestamp: 2025-08-13T10:24:01.545Z
Learning: The clang-tools-pip project is at version 0.x and primarily designed for use in cpp-linter/cpp-linter-action, so backward compatibility concerns for broader tooling ecosystem are not a priority.
Applied to files:
docs/wheel_cli_args.rstpyproject.tomlREADME.rst
🧬 Code graph analysis (3)
tests/test_wheel.py (1)
clang_tools/wheel.py (1)
main(22-31)
clang_tools/wheel.py (1)
tests/test_main.py (1)
parser(22-24)
docs/conf.py (2)
clang_tools/wheel.py (1)
get_parser(5-19)clang_tools/main.py (1)
get_parser(15-62)
🪛 Ruff (0.14.2)
tests/test_wheel.py
9-9: Unused lambda argument: tool
(ARG005)
9-9: Unused lambda argument: version
(ARG005)
21-21: Unused lambda argument: tool
(ARG005)
21-21: Unused lambda argument: version
(ARG005)
34-34: Unused lambda argument: tool
(ARG005)
34-34: 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). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (13)
docs/usage.rst (1)
1-2: Documentation polish is good.The header renaming improves conciseness without losing clarity.
.github/workflows/test.yml (1)
150-155: Wheel installation steps look reasonable.The clang-tools-wheel commands are executed for versions >= 13 with appropriate tool and version parameters. Ensure the wheel CLI implementation handles error cases gracefully (network failures, missing wheels, etc.).
README.rst (1)
1-181: Documentation expansion is comprehensive and well-organized.The README now clearly distinguishes binary installation from wheel installation with concrete examples and usage notes. The caveat about wheel CLI being intended primarily for cpp-linter projects (lines 142–143) appropriately manages expectations.
Note: Ensure the referenced CLI documentation links (line 95:
cli_args.html) and PyPI package links for wheels remain accurate as the project evolves.docs/index.rst (1)
14-14: Navigation change is correct.Adding
wheel_cli_argsto the toctree appropriately exposes the clang-tools-wheel CLI documentation alongside the main CLI docs.docs/api.rst (1)
12-14: API documentation addition is consistent with existing pattern.The automodule directive for clang_tools.wheel follows the same pattern as other modules and will auto-generate documentation from module docstrings.
Ensure
clang_tools/wheel.pyis properly documented with docstrings forget_parser()andmain()so that Sphinx can auto-generate the API reference.docs/wheel_cli_args.rst (1)
1-20: CLI documentation is clear and complete.The documentation concisely covers the clang-tools-wheel command options with appropriate version badges and descriptions.
pyproject.toml (1)
42-42: Console script entry looks correct.The clang-tools-wheel entry point properly references
clang_tools.wheel:main, following the same pattern as the existing clang-tools entry (line 41).Ensure
clang_tools/wheel.pyexports amain()function with the correct signature (typicallymain(argv=None) -> intor similar) that the console script can invoke..pre-commit-config.yaml (1)
19-21: Hook ID confirmed for ruff-pre-commit v0.14.3.The
ruff-checkhook ID is correct for v0.14.3, withruffavailable as a legacy alias. The code change is valid and future-proof.clang_tools/wheel.py (2)
34-35: LGTM!The entry point follows Python best practices for CLI scripts.
2-2: Acknowledge private function dependency and coupling risk.The import of
_resolve_installfromcpp_linter_hooks.utilis confirmed to be a private function with no documented public API alternative. This creates coupling between clang-tools-pip and cpp-linter-hooks internals. While this appears to be intentional for this v0.x utility (as the code and test suite show), the fragility risk should be acknowledged—changes tocpp_linter_hooksinternals could break this installer without notice. Document this coupling or consider monitoring cpp-linter-hooks releases for compatibility impacts.docs/conf.py (2)
17-17: LGTM!The import aliasing is appropriate to avoid naming conflicts with the existing
get_parserimport.
252-256: LGTM!Both documentation generation calls are correctly configured with appropriate parsers and program names.
tests/test_wheel.py (1)
7-10: Static analysis warnings about unused lambda arguments are false positives.The Ruff warnings about unused
toolandversionparameters in the mock lambdas are expected. Mock functions must match the signature of the real_resolve_installfunction, even if the test doesn't use all parameters. This is standard practice in testing.Also applies to: 20-22, 32-35
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
clang_tools/wheel.py (2)
22-32: Add exception handling for better error messages.The call to
_resolve_installon line 25 is not wrapped in a try-except block. If it raises an exception (e.g., network errors, invalid version format, etc.), the CLI will crash with a stack trace instead of providing a user-friendly error message.Apply this diff to add exception handling:
def main() -> int: parser = get_parser() args = parser.parse_args() - path = _resolve_install(args.tool, args.version) version_str = f" version {args.version}" if args.version else " latest version" + try: + path = _resolve_install(args.tool, args.version) + except Exception as e: + print(f"Failed to install {args.tool}{version_str}: {e}") + return 1 if path: print(f"{args.tool}{version_str} installed at: {path}") return 0 else: print(f"Failed to install {args.tool}{version_str}") return 1
26-26: Consider aligning version wording with help text.Line 26 uses "latest version" while the help text on line 17 uses "latest compatible version". For consistency, consider using the same wording.
Apply this diff:
- version_str = f" version {args.version}" if args.version else " latest version" + version_str = f" version {args.version}" if args.version else " latest compatible version"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
clang_tools/wheel.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
clang_tools/wheel.py (1)
tests/test_main.py (1)
parser(22-24)
⏰ 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). (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: install (20, windows-latest)
- GitHub Check: install (20, macos-13)
- GitHub Check: install (21, windows-latest)
- GitHub Check: install (19, macos-13)
- GitHub Check: install (17, windows-latest)
- GitHub Check: install (12.0.1, macos-13)
🔇 Additional comments (3)
clang_tools/wheel.py (3)
5-19: LGTM!The
get_parser()function is well-structured with appropriate argument defaults, choices, and help text. The previous docstring typo has been corrected.
35-36: LGTM!The entry point uses the standard Python pattern with proper exit code handling via
SystemExit.
2-2: Importing undocumented private API_resolve_installcreates maintenance risk.The import of
_resolve_installfromcpp_linter_hooks.util(line 2) uses a private API indicated by the leading underscore convention. Verification confirms this function is not documented in the cpp-linter-hooks public API or README, and no public alternative is exposed for wheel installation. This creates forward compatibility risk—the function could change or be removed in any version update without notice.Recommended actions:
- Check if cpp-linter-hooks exposes a public installation function or API
- If none exists, consider:
- Contributing a public API to cpp-linter-hooks for wheel installation
- Implementing wheel installation logic directly in this tool
- Documenting the private API dependency with clear version pinning
⛔ Skipped due to learnings
Learnt from: 2bndy5 Repo: cpp-linter/clang-tools-pip PR: 122 File: pyproject.toml:95-98 Timestamp: 2025-08-13T10:24:01.545Z Learning: The clang-tools-pip project is at version 0.x and primarily designed for use in cpp-linter/cpp-linter-action, so backward compatibility concerns for broader tooling ecosystem are not a priority.
|
To simplify usage and reduce maintenance effort, I didn’t modify the original CLI and instead created a new one. Tagging you @2bndy5 before moving forward in case you have any comments or suggestions. 👻 |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/test_wheel.py (1)
28-37: Clarify test purpose or rename it.The test name
test_main_default_toolis misleading because:
- Line 35 explicitly provides
--tool clang-format, so no default behavior is actually tested- The comment on Line 34 acknowledges "The CLI requires --tool"
- This test appears functionally similar to
test_main_success(both provide explicit tool and expect exit code 0)Consider renaming this test to reflect what it actually tests (e.g.,
test_main_with_minimal_argsif testing without--version) or removing it if it's redundant with existing coverage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/test.yml(5 hunks)README.rst(5 hunks)clang_tools/wheel.py(1 hunks)pyproject.toml(1 hunks)tests/test_wheel.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-13T10:24:01.545Z
Learnt from: 2bndy5
Repo: cpp-linter/clang-tools-pip PR: 122
File: pyproject.toml:95-98
Timestamp: 2025-08-13T10:24:01.545Z
Learning: The clang-tools-pip project is at version 0.x and primarily designed for use in cpp-linter/cpp-linter-action, so backward compatibility concerns for broader tooling ecosystem are not a priority.
Applied to files:
.github/workflows/test.ymlREADME.rst
🧬 Code graph analysis (2)
clang_tools/wheel.py (1)
tests/test_main.py (1)
parser(22-24)
tests/test_wheel.py (1)
clang_tools/wheel.py (1)
main(23-33)
🪛 Ruff (0.14.2)
tests/test_wheel.py
9-9: Unused lambda argument: tool
(ARG005)
9-9: Unused lambda argument: version
(ARG005)
20-20: Unused lambda argument: tool
(ARG005)
20-20: Unused lambda argument: version
(ARG005)
32-32: Unused lambda argument: tool
(ARG005)
32-32: 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). (2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: codeql / Analyze (python)
🔇 Additional comments (10)
clang_tools/wheel.py (1)
23-33: LGTM!The
main()function correctly handles both success and failure cases, with clear messaging and appropriate exit codes. The version string formatting (Line 27) properly addresses the previous review feedback about handlingNonevalues.tests/test_wheel.py (2)
5-15: LGTM!The test correctly mocks
resolve_installand verifies successful installation returns exit code 0.
18-25: LGTM!The test correctly validates that a failed installation (when
resolve_installreturnsNone) results in exit code 1..github/workflows/test.yml (4)
71-71: LGTM!The more specific artifact path pattern
dist/clang_tools*.whlclearly identifies the package wheels being uploaded.
101-101: LGTM!The renamed step titles clearly distinguish binary checks from the new wheel checks, improving workflow readability.
Also applies to: 116-116, 135-135
150-170: LGTM!The clang-format wheel installation and verification step is well-structured with:
- Proper fail-fast behavior (
set -e)- Clear version output for debugging
- Explicit version verification with helpful error messages
173-191: LGTM!The clang-tidy wheel verification logic mirrors the clang-format step and correctly validates the installed version (assuming the conditional on Line 172 works as intended).
README.rst (3)
7-9: LGTM!The badge updates and description changes accurately reflect the new wheel installation capabilities, with clear communication about which tools support wheels.
Also applies to: 26-26, 34-34
99-136: LGTM!The installation examples are clear and practical. The important note (Lines 140-146) appropriately communicates that
clang-tools-wheelis primarily intended for cpp-linter projects while recommending standard tools for general use.Also applies to: 140-146
165-187: LGTM!The supported versions documentation is well-organized, clearly distinguishing between binary and wheel support with helpful PyPI references for users to check available versions.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
clang_tools/wheel.py (1)
8-13: Consider makingtoola positional argument for cleaner CLI syntax.As discussed in previous reviews, making
toolpositional would simplify the command interface fromclang-tools-wheel --tool clang-format --version 21toclang-tools-wheel clang-format --version 21, following common CLI patterns.Apply this diff to implement the change:
parser.add_argument( - "--tool", - required=True, + "tool", choices=["clang-format", "clang-tidy"], help="Tool to install (clang-format or clang-tidy)", )Note: This would also require updating the documentation examples in README.rst to match the new syntax.
README.rst (1)
26-27: Consider minor rewording for clarity.The current phrasing "clang-format, clang-tidy, clang-query, and clang-apply-replacements static binaries or Python wheels" could briefly suggest all four tools are available as both binaries and wheels. While line 34 clarifies that wheels only support clang-format and clang-tidy, you might consider rewording for immediate clarity.
For example:
-Easily install clang-format, clang-tidy, clang-query, and clang-apply-replacements static binaries or Python wheels using the ``clang-tools`` CLI. +Easily install clang-format, clang-tidy, clang-query, and clang-apply-replacements static binaries using the ``clang-tools`` CLI. Python wheels are also supported for clang-format and clang-tidy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.rst(5 hunks)clang_tools/wheel.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-13T10:24:01.545Z
Learnt from: 2bndy5
Repo: cpp-linter/clang-tools-pip PR: 122
File: pyproject.toml:95-98
Timestamp: 2025-08-13T10:24:01.545Z
Learning: The clang-tools-pip project is at version 0.x and primarily designed for use in cpp-linter/cpp-linter-action, so backward compatibility concerns for broader tooling ecosystem are not a priority.
Applied to files:
README.rst
🧬 Code graph analysis (1)
clang_tools/wheel.py (1)
tests/test_main.py (1)
parser(22-24)
⏰ 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: install (21, macos-13)
- GitHub Check: install (19, macos-13)
- GitHub Check: install (16, windows-latest)
- GitHub Check: install (16, macos-13)
- GitHub Check: install (13, macos-13)
🔇 Additional comments (3)
clang_tools/wheel.py (1)
1-36: LGTM! Clean implementation of the wheel installation CLI.The code correctly:
- Integrates with
cpp_linter_hooks.util.resolve_install- Handles the optional version parameter with appropriate default messaging
- Provides clear success/failure output with proper exit codes
- Follows standard CLI entry point patterns
All previously flagged issues (typo, unreachable default, error message formatting) have been properly addressed.
README.rst (2)
150-158: Verify examples align with CLI implementation.The examples correctly match the current CLI implementation where
--toolis a required flag. If you implement the suggested refactor to maketoola positional argument inwheel.py, remember to update these examples accordingly.For example, if the positional argument change is made, update to:
# Install latest clang-format wheel clang-tools-wheel clang-format # Install specific version clang-format wheel clang-tools-wheel clang-format --version 21
1-188: Excellent documentation updates!The documentation clearly:
- Distinguishes between binary and wheel installation workflows
- Provides helpful context about the intended audience for
clang-tools-wheel- Includes practical examples for both installation methods
- Organizes version support information in an easy-to-scan format
- Links to relevant external resources (PyPI pages)
The restructured sections make it easy for users to find the installation method they need.
shenxianpeng
force-pushed
the
support-wheel-install
branch
from
4d6a9ca to
375b725
Compare
shenxianpeng
force-pushed
the
support-wheel-install
branch
from
375b725 to
8b0c0cb
Compare
shenxianpeng
changed the title
shenxianpeng
changed the title
shenxianpeng
force-pushed
the
support-wheel-install
branch
from
8b0c0cb to
1ab92d5
Compare
github-actions
bot
added
the
documentation
|
Replace cpp-linter/cpp-linter-hooks#136 and address cpp-linter/cpp-linter-action#364
I didn’t modify the original CLI; instead, I created a new one to simplify usage and reduce maintenance effort.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores