clang-format hook support --verbose output
#79
Conversation
github-actions
bot
added
the
documentation
|
Warning Rate limit exceeded@shenxianpeng has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 55 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 (3)
## Walkthrough
A verbose mode was added to the clang-format pre-commit hook by introducing a `--verbose` flag to the hook's argument parser. Documentation and testing updates were made to guide users on enabling and verifying verbose output, including a new troubleshooting section in the README and corresponding test cases.
## Changes
| File(s) | Change Summary |
|---------------------------------------------------|---------------------------------------------------------------------------------------------------|
| README.md | Added troubleshooting section with instructions for enabling verbose output in clang-format hook.|
| cpp_linter_hooks/clang_format.py | Added `-v`/`--verbose` flag to argument parser for verbose output support; updated command execution and output handling. |
| testing/pre-commit-config-verbose.yaml | New pre-commit config testing clang-format with `--verbose` enabled using both `--verbose` and `-v`.|
| testing/run.sh | Updated to include new verbose test config in pre-commit test sequence with explicit runs and result aggregation. |
| tests/test_clang_format.py | Added two tests for `run_clang_format` with the `--verbose` flag, covering success and error cases.|
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant PreCommit
participant ClangFormatHook
participant ClangFormat
User->>PreCommit: Run pre-commit with --all-files
PreCommit->>ClangFormatHook: Execute clang-format hook (with --verbose)
ClangFormatHook->>ClangFormat: Run clang-format with --verbose
ClangFormat-->>ClangFormatHook: Output processed files (verbose)
ClangFormatHook-->>PreCommit: Return result, including verbose output
PreCommit-->>User: Show hook results and verbose detailsAssessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes were found. |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.md(1 hunks)cpp_linter_hooks/clang_format.py(1 hunks)testing/pre-commit-config-verbose.yaml(1 hunks)testing/run.sh(1 hunks)tests/test_clang_format.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Test
tests/test_clang_format.py
[error] 98-98: Test failure: 'test_run_clang_format_verbose_error' failed due to assertion error. Expected error message 'Invalid value for -style' not found in output despite clang-format reporting 'Invalid value for -style'.
🔇 Additional comments (4)
testing/run.sh (1)
4-4: Good addition to test coverage.The script correctly includes the new verbose configuration file in the test loop, ensuring the verbose functionality is tested alongside existing configurations.
testing/pre-commit-config-verbose.yaml (1)
1-6: Well-structured configuration file.The pre-commit configuration correctly includes the
--verboseflag and follows the standard format. This will be useful for testing verbose output functionality once it's properly implemented.README.md (1)
154-167: Excellent documentation addition.The troubleshooting section provides clear guidance on enabling verbose output for debugging clang-format issues. The example configuration is helpful and matches the intended usage pattern.
tests/test_clang_format.py (1)
69-83: Test may pass for the wrong reasons.This test verifies that the verbose flag doesn't break the formatting functionality, but it doesn't actually verify that verbose output is produced since the verbose flag isn't implemented in the main code.
Once the verbose functionality is properly implemented in
clang_format.py, this test should be enhanced to verify that verbose output is actually generated.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cpp_linter_hooks/clang_format.py (1)
39-44: Consider simplifying the dry-run handling logic.The dry-run handling works correctly but could be simplified using a ternary operator as suggested by static analysis.
Apply this diff to simplify the logic:
- # Handle special case for dry-run mode - if "--dry-run" in command: - retval = -1 # Special code to identify dry-run mode - else: - retval = sp.returncode + # Handle special case for dry-run mode + retval = -1 if "--dry-run" in command else sp.returncode
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cpp_linter_hooks/clang_format.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
cpp_linter_hooks/clang_format.py (3)
cpp_linter_hooks/util.py (1)
ensure_installed(38-51)cpp_linter_hooks/clang_tidy.py (1)
main(32-36)testing/good.c (1)
main(2-6)
🪛 Ruff (0.11.9)
cpp_linter_hooks/clang_format.py
40-43: Use ternary operator retval = -1 if "--dry-run" in command else sp.returncode instead of if-else-block
(SIM108)
🔇 Additional comments (8)
cpp_linter_hooks/clang_format.py (8)
2-2: LGTM: Import needed for verbose output.The
sysimport is correctly added to support printing verbose information tostderr.
11-13: LGTM: Verbose flag properly implemented.The verbose argument is correctly added with both short (
-v) and long (--verbose) forms, addressing the previous review comment about the flag being defined but never used.
20-24: LGTM: Verbose flag correctly passed to clang-format.The implementation properly checks the parsed verbose flag and appends
--verboseto the clang-format command when requested.
28-37: LGTM: Output capture enhanced for verbose mode.The subprocess call correctly captures both
stdoutandstderr, combining them for comprehensive output handling. This enables proper verbose output display and error reporting.
45-48: LGTM: Verbose output properly implemented.The verbose information is correctly printed when the verbose flag is enabled, providing useful debugging information to users.
51-53: LGTM: Exception handling improved.The explicit
FileNotFoundErrorhandling provides clearer error reporting and is more specific than generic exception handling.
55-61: LGTM: Well-designed verbose output function.The helper function appropriately prints debugging information to
stderrand handles empty output gracefully. The function design is clean and focused.
66-71: LGTM: Main function properly handles dry-run mode.The main function correctly handles the dry-run special case by excluding the sentinel return code from error output and converting it to a success return code.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #79 +/- ##
==========================================
+ Coverage 89.61% 95.12% +5.51%
==========================================
Files 3 3
Lines 77 82 +5
==========================================
+ Hits 69 78 +9
+ Misses 8 4 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
shenxianpeng
changed the title
--verbose output
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
README.md (3)
133-135: Prefer “for” instead of “of” in the note sentenceThe phrase reads a bit awkwardly. Re-wording improves clarity and matches common pre-commit phrasing.
-> [!NOTE] -> Use `--dry-run` in `args` of `clang-format` to print instead of changing the format, e.g.: +> [!NOTE] +> Use `--dry-run` in the `args` for `clang-format` to print the diff rather than change the files, e.g.:
63-66: Keepargslist style consistent across snippetsOther snippets present each flag as a separate list element (no commas inside the string). Using the same style here avoids confusion about whether the comma is part of the argument.
- args: [--style=file, --version=18] # Specifies version + args: [--style=file, --version=18] # two separate list items – comma separates YAML list, not the argument string
95-106: Call out that-v/--verboseis mutually exclusive with--quietIf the hook already exposes a
--quietflag, it’s worth noting here that the two are mutually exclusive to avoid user confusion.Consider appending a brief sentence such as:
_Note: `--verbose` cannot be combined with `--quiet`; the last flag wins._
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(8 hunks)testing/run.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- testing/run.sh
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~134-~134: “of” seems less likely than “or” (‘either … or’).
Context: ...> [!NOTE] > Use--dry-runinargsofclang-format` to print instead of chan...
(AI_HYDRA_LEO_CP_OF_OR)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test (3.14)
- GitHub Check: test (3.12)
shenxianpeng
force-pushed
the
support-verbose
branch
from
d9e6102 to
e0b7980
Compare
|
closes #77
Summary by CodeRabbit
New Features
Documentation
Tests