Add verbose #78
Add verbose #78
Conversation
github-actions
bot
added
the
documentation
|
WalkthroughVerbose output and debugging support were added to the clang-format and clang-tidy hooks, including new command-line flags, utility functions for verbose subprocess execution, and expanded documentation. Example configurations and tests for verbose/debug behavior were introduced. The pre-commit testing script and configuration files were updated to reflect and test these enhancements. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PreCommit
participant HookScript
participant Util
User->>PreCommit: Runs pre-commit with --verbose
PreCommit->>HookScript: Executes clang-format/clang-tidy hook with --verbose
HookScript->>Util: Calls run_subprocess_with_logging(command, tool_name, verbose)
Util->>HookScript: Returns (exit_code, output)
HookScript->>PreCommit: Prints debug info (if verbose), returns result
PreCommit->>User: Displays detailed output and exit code
Suggested labels
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 1
🧹 Nitpick comments (1)
cpp_linter_hooks/util.py (1)
75-81: Simplify nested if statements as suggested by static analysis.The nested if statements can be combined for better readability.
- # Special handling for clang-tidy warnings/errors - if tool_name == "clang-tidy" and ( - "warning:" in combined_output or "error:" in combined_output - ): - if retval == 0: # Only override if it was successful - retval = 1 - debug_print("Found warnings/errors, setting exit code to 1", verbose) + # Special handling for clang-tidy warnings/errors + if (tool_name == "clang-tidy" and + ("warning:" in combined_output or "error:" in combined_output) and + retval == 0): # Only override if it was successful + retval = 1 + debug_print("Found warnings/errors, setting exit code to 1", verbose)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
README.md(1 hunks)cpp_linter_hooks/clang_format.py(2 hunks)cpp_linter_hooks/clang_tidy.py(2 hunks)cpp_linter_hooks/util.py(2 hunks)testing/pre-commit-config-verbose.yaml(1 hunks)testing/run.sh(1 hunks)tests/test_clang_format.py(2 hunks)tests/test_clang_tidy.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
tests/test_clang_format.py (3)
cpp_linter_hooks/clang_format.py (2)
run_clang_format(19-37)main(40-44)cpp_linter_hooks/clang_tidy.py (1)
main(37-41)tests/test_clang_tidy.py (1)
test_verbose_output(68-94)
tests/test_clang_tidy.py (2)
tests/test_clang_format.py (1)
test_verbose_output(86-112)cpp_linter_hooks/clang_tidy.py (1)
run_clang_tidy(19-34)
cpp_linter_hooks/clang_tidy.py (1)
cpp_linter_hooks/util.py (3)
ensure_installed(113-126)debug_print(17-20)run_subprocess_with_logging(23-88)
🪛 Ruff (0.11.9)
cpp_linter_hooks/util.py
75-78: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
🪛 GitHub Actions: Test
tests/test_clang_tidy.py
[error] 61-61: Test failure: test_main_as_script did not raise SystemExit as expected.
🔇 Additional comments (22)
cpp_linter_hooks/util.py (2)
17-21: LGTM! Clean and focused utility function.The
debug_printfunction is well-implemented with a clear purpose and simple interface.
23-89: Comprehensive subprocess utility with solid error handling.The
run_subprocess_with_loggingfunction provides excellent centralized handling for subprocess execution with verbose logging, dry-run support, and tool-specific behavior. The implementation correctly handles:
- Subprocess execution with proper output capture
- Dry-run mode for clang-format
- Special exit code handling for clang-tidy warnings/errors
- Comprehensive error handling
testing/run.sh (1)
4-4: Good addition to test the new verbose configuration.Adding the verbose configuration file to the test loop ensures the new verbose functionality is properly exercised during testing.
README.md (1)
84-121: Excellent documentation for the new verbose debugging features.The new "Verbose Output and Debugging" section provides comprehensive guidance for users:
- Clear configuration examples with verbose flags
- Detailed explanation of verbose output contents
- Practical debugging scenarios and use cases
- Proper command-line examples
This documentation will be very helpful for users troubleshooting issues with the hooks.
testing/pre-commit-config-verbose.yaml (1)
1-26: Well-structured example configuration with excellent documentation.This verbose configuration file provides:
- Complete working example with both clang-format and clang-tidy
- Clear comments explaining usage and benefits
- Specific guidance for troubleshooting (e.g., exit code 247)
- Proper file patterns and argument combinations
This will be very useful for users who need to debug hook issues.
tests/test_clang_format.py (3)
70-83: Good test coverage for the main() function edge case.The
test_main_empty_outputtest properly validates the behavior when clang-format returns an error with empty output, ensuring the main function handles this scenario correctly.
86-113: Comprehensive test for verbose mode functionality.The
test_verbose_outputtest effectively verifies:
- Debug messages are printed to stderr when verbose mode is enabled
- The subprocess utility is called with correct parameters
- Mocking is properly used to isolate the functionality
This ensures the verbose feature works as expected.
115-169: Excellent test coverage for error conditions and dry-run mode.The additional verbose tests (
test_verbose_with_errorandtest_verbose_dry_run) provide comprehensive coverage of:
- Error handling in verbose mode
- Dry-run mode behavior with verbose output
- Proper return value validation
- Correct subprocess utility integration
These tests ensure robust behavior across different scenarios.
tests/test_clang_tidy.py (4)
4-4: Good addition of necessary import.The
patchimport is correctly added to support the new test functions that use mocking.
68-95: Excellent test coverage for verbose output.This test properly mocks the necessary functions and verifies that verbose mode produces the expected debug output. The test structure and assertions are well-designed.
97-126: Good test coverage for verbose mode with warnings.The test correctly simulates the scenario where clang-tidy returns warnings and verifies that the output is properly handled and return codes are correctly propagated.
128-151: Good test coverage for file not found scenarios.The test properly simulates the FileNotFoundError scenario and verifies the error handling behavior in verbose mode.
cpp_linter_hooks/clang_tidy.py (5)
4-9: Good refactoring to use centralized utility functions.The imports have been updated to use the new utility functions from
util.py, which centralizes subprocess handling and verbose logging functionality.
14-16: Proper addition of verbose argument.The verbose argument is correctly added with appropriate short and long forms, following standard CLI conventions.
25-34: Excellent refactoring with proper verbose support.The function has been well-refactored to:
- Extract the verbose flag from arguments
- Add debug logging for version and executable path
- Use the centralized
run_subprocess_with_loggingutility function- Pass the correct parameters including
dry_run=Falsefor clang-tidyThis maintains the same functionality while adding verbose support and improving code consistency.
39-40: Appropriate use of pragma no cover comments.The pragma comments are correctly applied to the error handling code in the main function, which is typically not covered in unit tests.
44-44: Appropriate use of pragma no cover comment.The pragma comment is correctly applied to the main guard, which is typically not covered in unit tests.
cpp_linter_hooks/clang_format.py (5)
4-9: Good refactoring to use centralized utility functions.The imports have been updated to use the new utility functions from
util.py, maintaining consistency with the clang_tidy.py refactoring.
14-16: Proper addition of verbose argument.The verbose argument is correctly added with the same format as clang_tidy.py, ensuring consistency across the codebase.
25-37: Excellent refactoring with proper verbose and dry-run support.The function has been well-refactored to:
- Extract the verbose flag from arguments
- Add debug logging for version and executable path
- Detect dry-run mode from the command arguments
- Use the centralized
run_subprocess_with_loggingutility function- Pass the correct parameters including the dry-run flag
This maintains the same functionality while adding verbose support and improving code consistency with clang_tidy.py.
42-43: Appropriate use of pragma no cover comments.The pragma comments are correctly applied to the error handling code in the main function, consistent with the clang_tidy.py implementation.
47-47: Appropriate use of pragma no cover comment.The pragma comment is correctly applied to the main guard, consistent with the clang_tidy.py implementation.
| def test_main_as_script(): | ||
| """Test the if __name__ == '__main__' behavior""" | ||
| with patch("cpp_linter_hooks.clang_tidy.main") as mock_main: | ||
| mock_main.return_value = 42 | ||
|
|
||
| # This would normally raise SystemExit, but we're mocking main() | ||
| with pytest.raises(SystemExit) as exc_info: | ||
| # Simulate running the script directly | ||
| exec("if __name__ == '__main__': raise SystemExit(main())") | ||
|
|
||
| assert exc_info.value.code == 42 | ||
|
|
There was a problem hiding this comment.
Fix the test logic for main script execution.
The test is failing because it's not actually testing the module's if __name__ == '__main__' behavior. The exec() statement executes a generic condition that doesn't relate to the clang_tidy module.
-def test_main_as_script():
- """Test the if __name__ == '__main__' behavior"""
- with patch("cpp_linter_hooks.clang_tidy.main") as mock_main:
- mock_main.return_value = 42
-
- # This would normally raise SystemExit, but we're mocking main()
- with pytest.raises(SystemExit) as exc_info:
- # Simulate running the script directly
- exec("if __name__ == '__main__': raise SystemExit(main())")
-
- assert exc_info.value.code == 42
+def test_main_as_script():
+ """Test the if __name__ == '__main__' behavior"""
+ with patch("cpp_linter_hooks.clang_tidy.main") as mock_main:
+ mock_main.return_value = 42
+
+ # Import the module to trigger the if __name__ == '__main__' check
+ with patch("cpp_linter_hooks.clang_tidy.__name__", "__main__"):
+ with pytest.raises(SystemExit) as exc_info:
+ import cpp_linter_hooks.clang_tidy
+
+ assert exc_info.value.code == 42Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: Test
[error] 61-61: Test failure: test_main_as_script did not raise SystemExit as expected.
🤖 Prompt for AI Agents
In tests/test_clang_tidy.py around lines 55 to 66, the test incorrectly uses
exec() with a generic if __name__ == '__main__' condition that does not invoke
the actual clang_tidy module's main function. To fix this, modify the test to
execute the clang_tidy module as a script, for example by using runpy.run_module
or a similar approach that triggers the module's real __main__ behavior,
ensuring the test properly verifies the module's script execution and SystemExit
with the mocked main return value.
Summary by CodeRabbit
New Features
Documentation
Tests