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 #97 +/- ##
=======================================
Coverage 94.48% 94.48%
=======================================
Files 3 3
Lines 145 145
=======================================
Hits 137 137
Misses 8 8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
WalkthroughAdds clang-format patch/major versions (20.1.8, 21.1.0), updates optional tooling pin to 21.1.0, expands tests to cover clang-format/clang-tidy v21 and resolves 20/20.1 → 20.1.8, bumps two pre-commit hook revisions, and updates README and migration notes. Changes
Sequence Diagram(s)(Skipped — changes are data/tests/docs/version bumps; no control-flow or feature-level interactions to diagram.) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Out-of-scope changes
Possibly related PRs
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
cpp_linter_hooks/util.py (1)
163-172: Handle version=None installs gracefully to avoid “==None”.If defaults are None (your tests simulate this), _install_tool would attempt to pip install f"{tool}==None". Instead, install unpinned when version is None.
Apply this diff:
def _install_tool(tool: str, version: str) -> Optional[Path]: """Install a tool using pip.""" try: - subprocess.check_call( - [sys.executable, "-m", "pip", "install", f"{tool}=={version}"] - ) + cmd = ( + [sys.executable, "-m", "pip", "install", f"{tool}=={version}"] + if version + else [sys.executable, "-m", "pip", "install", tool] + ) + subprocess.check_call(cmd) return shutil.which(tool)Follow-up: adjust tests that assert the exact pip args when version is None.
tests/test_util.py (2)
36-39: Fix runtime-version mocking to avoid unintended installs during tests.When version is None, DEFAULT_CLANG_FORMAT_VERSION is now 21.1.0; when version is "20", _resolve_version(...) is 20.1.8. Mock the runtime accordingly to prevent triggering _install_tool.
Apply this diff:
- # Mock _get_runtime_version to return a matching version - mock_version = "20.1.7" if tool == "clang-format" else "20.1.0" + # Mock _get_runtime_version to return a matching version for each case + if tool == "clang-format": + mock_version = ( + _resolve_version(CLANG_FORMAT_VERSIONS, "20") + if version == "20" + else DEFAULT_CLANG_FORMAT_VERSION + ) + else: + mock_version = DEFAULT_CLANG_TIDY_VERSION
168-173: Update exact-match expectations after removing 20.1.7.Given CLANG_FORMAT_VERSIONS no longer contains 20.1.7, the exact-match test will fail. Either switch the exact-match to 20.1.8, or assert that 20.1.7 resolves to None to document the removal.
Apply one of the following diffs.
Option A (keep exact-match semantics):
- ("20.1.7", "20.1.7"), # Exact match + ("20.1.8", "20.1.8"), # Exact matchOption B (document removal):
- ("20.1.7", "20.1.7"), # Exact match + ("20.1.7", None), # Removed from supported versions
♻️ Duplicate comments (1)
tests/test_clang_tidy.py (1)
47-48: Duplicate of the valid case note above.
🧹 Nitpick comments (9)
pyproject.toml (1)
51-53: Approve bump to clang-format 21.1.0; no test updates needed. Tests stub the runtime version via_get_runtime_version, so they won’t break with the new default.
Clang-tidy remains at 20.1.0—consider bumping it to 21.x for parity with “support clang v21,” or explicitly scope this PR to clang-format only.tests/test_clang_tidy.py (2)
25-26: Adding v21 arg coverage is fine; confirm install behavior expectation.With CLANG_TIDY_VERSIONS capped at 20.1.0, asking for “--version=21” will fall back to DEFAULT_CLANG_TIDY_VERSION at runtime. If the intent is true v21 support, add 21.x to CLANG_TIDY_VERSIONS (and optionally pin in pyproject). If fallback is intended, consider a dedicated test asserting fallback semantics.
9-13: Nit: duplicate CMake invocation.The two consecutive cmake -Bbuild calls are redundant.
Apply this diff:
def generate_compilation_database(): subprocess.run(["mkdir", "-p", "build"]) - subprocess.run(["cmake", "-Bbuild", "testing/"]) subprocess.run(["cmake", "-Bbuild", "testing/"])cpp_linter_hooks/util.py (1)
105-107: Version table updated; address test fallout and consider retaining 20.1.7 for back-compat.
- Removing 20.1.7 breaks the exact-match test in tests/test_util.py (it still expects "20.1.7" → "20.1.7"). Either:
- Re-add "20.1.7" here for back-compat, or
- Update the test to expect None (or switch the exact-match case to 20.1.8). I’ve proposed a test fix below.
Apply this diff if you prefer back-compat:
"20.1.6", + "20.1.7", "20.1.8", "21.1.0",tests/test_util.py (1)
352-357: If you adopt unpinned install for version=None, adjust this assertion.After updating _install_tool to handle None, the call expectation here should match the new behavior (i.e., pip install clang-format without ==version). If you keep current behavior, ignore this note.
tests/test_clang_format.py (3)
17-18: Add clang-format v21 happy-path — LGTM. Also cover patch-level.Looks good. To harden against resolver regressions, consider adding 21.1 and 21.1.0 cases:
(["--style=Google", "--version=21"], (0, "")), + (["--style=Google", "--version=21.1"], (0, "")), + (["--style=Google", "--version=21.1.0"], (0, "")),
44-45: Add v21 to invalid-path matrix — LGTM. Mirror patch-level too.For symmetry with the valid matrix and to exercise error handling across patch resolution:
(["--style=Google", "--version=20"], 1), (["--style=Google", "--version=21"], 1), + (["--style=Google", "--version=21.1"], 1), + (["--style=Google", "--version=21.1.0"], 1),
8-18: Parametrize tests usingCLANG_FORMAT_VERSIONSfrom util
ImportCLANG_FORMAT_VERSIONSfromcpp_linter_hooks.utiland derive the sorted set of major versions in tests/test_clang_format.py (lines 8–18) instead of hardcoding the list to avoid drift when versions change.README.md (1)
62-70: Surface v21 in examples (optional).Since v21 is now supported, consider showcasing it in snippets to guide users:
# Custom Clang Tool Version - id: clang-format args: [--style=file, --version=21] - id: clang-tidy args: [--checks=.clang-tidy, --version=21]# Verbose example - id: clang-format args: [--style=file, --version=21, --verbose]Also applies to: 178-184
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
.pre-commit-config.yaml(2 hunks)README.md(6 hunks)cpp_linter_hooks/util.py(1 hunks)pyproject.toml(1 hunks)tests/test_clang_format.py(2 hunks)tests/test_clang_tidy.py(2 hunks)tests/test_util.py(1 hunks)
🔇 Additional comments (9)
.pre-commit-config.yaml (2)
3-3: Pre-commit hooks bump: LGTM.
17-17: Ruff pre-commit bump: LGTM.README.md (7)
35-35: Bump rev to v1.1.0 — LGTM.
50-50: Keep examples consistent — LGTM.
65-65: Rev updated in Custom Clang Tool Version block — LGTM.
73-73: Migration note clarity — LGTM.Wording correctly reflects the pre-commit hook using wheels.
151-151: Performance tip refinement — LGTM.
154-154: Rev bump maintained in performance example — LGTM.
181-181: Rev bump maintained in verbose example — LGTM.
CodSpeed Performance ReportMerging #97 will degrade performances by 11.12%Comparing Summary
Benchmarks breakdown
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
testing/pre-commit-config-verbose.yaml (2)
2-3: Prefer a fixed rev over HEAD in examples/tests to avoid flaky autoupdate warnings.Pin to a tag or commit unless HEAD is intentional for the test. If intentional, add a comment to silence confusion.
Apply:
- - repo: . - rev: HEAD + - repo: . + rev: v1.1.0 # Intentional: use a fixed tag to avoid mutable ref warnings
11-11: Same as above: ensure -v path is exercised.If not covered elsewhere, add a test that runs with "-v".
testing/pre-commit-config-version.yaml (1)
2-3: Avoid mutable rev: HEAD in public examples.Use a fixed tag to prevent pre-commit warnings and ensure reproducibility.
- - repo: . - rev: HEAD + - repo: . + rev: v1.1.0(Repeat for each block if these files are used as user-facing examples.)
README.md (5)
57-70: Update versioned example to showcase the newly supported 21.Align the sample with the PR goal (support clang v21).
- repo: https://github.com/cpp-linter/cpp-linter-hooks - rev: v1.1.0 + rev: v1.1.0 hooks: - id: clang-format - args: [--style=file, --version=18] # Specifies version + args: [--style=file, --version=21] # Specifies version - id: clang-tidy - args: [--checks=.clang-tidy, --version=18] # Specifies version + args: [--checks=.clang-tidy, --version=21] # Specifies version
72-74: Minor wording: clarify what --version does.“Loads style configuration via --version” is misleading elsewhere; --version selects tool version, not style.
Consider: “To select tool version, use --version. Style/checks are loaded via --style=file and --checks=.clang-tidy.”
149-162: Keep version examples consistent with v21 across docs.Optional but improves cohesion.
- rev: v1.1.0 + rev: v1.1.0 ... - - id: clang-format - args: [--style=file, --version=20] + - id: clang-format + args: [--style=file, --version=21] ... - - id: clang-tidy - args: [--checks=.clang-tidy, --version=20] + - id: clang-tidy + args: [--checks=.clang-tidy, --version=21]
178-184: Verbose example: bump to 21 for consistency.Keep examples uniform with newly added support.
- rev: v1.1.0 + rev: v1.1.0 ... - args: [--style=file, --version=18, --verbose] # Add -v or --verbose for detailed output + args: [--style=file, --version=21, --verbose] # Add -v or --verbose for detailed output
190-197: Table row phrasing: fix “Loads style configuration via --version”.Use accurate flags for style/checks loading.
-| Loads style configuration | ✅ via `--version` | ✅ (default behavior) | +| Loads style/checks configuration | ✅ via `--style=file` / `--checks=.clang-tidy` | ✅ (default behavior) |
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
README.md(6 hunks)testing/pre-commit-config-verbose.yaml(1 hunks)testing/pre-commit-config-version.yaml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
testing/pre-commit-config-verbose.yaml (1)
tests/test_clang_format.py (1)
test_run_clang_format_verbose(73-86)
🪛 GitHub Actions: Test
testing/pre-commit-config-verbose.yaml
[warning] 1-1: Warning: The 'rev' field of repo '.' appears to be a mutable reference; pre-commit autoupdate recommended.
testing/pre-commit-config-version.yaml
[warning] 1-1: Warning: The 'rev' field of repo '.' appears to be a mutable reference; pre-commit autoupdate recommended.
⏰ 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: Run benchmarks
🔇 Additional comments (2)
testing/pre-commit-config-verbose.yaml (1)
6-6: Cover both verbose flags in tests.This config exercises --verbose; add/confirm a test that exercises -v as well (mirrors the second block).
Do you want me to add a parametrized test for ["--verbose", "-v"] to test_run_clang_format_verbose?
README.md (1)
31-40: Pin the example to the latest released tag.Good update to v1.1.0. LGTM.
| - repo: . | ||
| rev: HEAD | ||
| hooks: | ||
| - id: clang-format | ||
| args: [--style=file, --version=21] | ||
| - id: clang-tidy | ||
| args: [--checks=.clang-tidy, --version=21] |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Validate clang-tidy 21 wheel availability and hook resolution.
You added --version=21 for clang-tidy, but optional deps remain at 20.1.0 in pyproject. Verify the hook downloads/uses the 21.x wheel at runtime on CI.
Run:
🏁 Script executed:
#!/bin/bash
# Check PyPI for clang-tidy 21 availability
curl -s https://pypi.org/pypi/clang-tidy/json | jq -r '.releases | keys[]' | rg '^21(\.|$)' -n
# Print latest version for visibility
curl -s https://pypi.org/pypi/clang-tidy/json | jq -r '.info.version'Length of output: 173
Align clang-tidy hook version with available wheel
PyPI only hosts clang-tidy up to version 20.1.0 (no 21.x releases), so --version=21 will fail at hook resolution; revert the hook to --version=20.1.0 (to match your optional deps) or wait for a 21.x wheel before bumping.
🤖 Prompt for AI Agents
In testing/pre-commit-config-version.yaml around lines 37 to 43, the clang-tidy
hook is pinned to --version=21 which is not available on PyPI; update the
clang-tidy hook to use --version=20.1.0 to match the available wheel (or
remove/bump only when a 21.x wheel exists). Edit the args for the clang-tidy
entry to replace --version=21 with --version=20.1.0 so pre-commit can resolve
the hook successfully.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
testing/run.sh (1)
32-32: Bumped expected failures to 10 — verify stability and harden the check.Confirm that 10 is the intended count across all CI environments; pre-commit output can vary slightly with hook versions. Also, prefer arithmetic test to avoid word-splitting/unset issues.
Apply this minimal change to the condition:
-if [ $failed_cases -eq 10 ]; then +if (( failed_cases == 10 )); thenOptional hardening (outside this line): initialize and clean up the results file and make grep safer.
# At the top, before first run: RESULT_FILE="$(mktemp)"; trap 'rm -f "$RESULT_FILE"' EXIT : > "$RESULT_FILE" # Replace result.txt usages with "$RESULT_FILE" pre-commit run ... | tee -a "$RESULT_FILE" || true failed_cases=$(grep -c -F "Failed" "$RESULT_FILE" || true)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
testing/run.sh(1 hunks)
⏰ 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: test (3.14)
- GitHub Check: Run benchmarks
shenxianpeng
removed
the
documentation
1 participant
closes #93 also
Summary by CodeRabbit
New Features
Documentation
Tests
Chores