feat: capture and output clang tool's version number #54
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #54 +/- ##
==========================================
+ Coverage 97.48% 97.54% +0.06%
==========================================
Files 14 14
Lines 3338 3385 +47
==========================================
+ Hits 3254 3302 +48
+ Misses 84 83 -1 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
This affects thread-comments, step-summary, and PR review summary. I also added tests to ensure PR review summary only shows relevant information about the tools that were actually used.
2bndy5
force-pushed
the
show-version-used
branch
from
377779d to
4c4f20a
Compare
WalkthroughThe pull request introduces significant modifications across several files in the cpp-linter project. Key changes include the simplification of the Changes
Possibly related PRs
Suggested labels
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 3
🧹 Outside diff range and nitpick comments (9)
cpp-linter/src/run.rs (1)
Line range hint
131-137: LGTM! Consider adding debug logging for version capture.The implementation correctly captures clang versions with proper error handling. However, since version information is crucial for debugging issues, consider adding debug logging before and after the version capture.
+ log::debug!("Capturing clang tool versions..."); let clang_versions = capture_clang_tools_output( &mut arc_files, cli.version.as_str(), &mut clang_params, &rest_api_client, ) .await?; + log::debug!("Captured clang versions: {:?}", clang_versions);cpp-linter/src/common_fs/mod.rs (1)
Line range hint
161-201: LGTM! Consider adding a descriptive comment for the total counter.The changes look good and improve the tracking of clang-tidy diagnostics. The string formatting changes enhance readability, and the Option handling for tool_total is safe.
Consider adding a brief comment explaining what the
totalcounter represents:+ // Count of clang-tidy diagnostics that had no fixes applied let mut total = 0;cpp-linter/tests/reviews.rs (2)
164-182: Consider extracting duplicated summary logic into a helper functionThe summary generation logic for tidy and format reviews is duplicated. Consider refactoring into a helper function to improve maintainability.
+ fn generate_tool_summary(review_enabled: bool, force_lgtm: bool, tool_name: &str) -> String { + if !review_enabled { + return String::new(); + } + if force_lgtm { + format!("No concerns reported by {}. Great job! :tada:", tool_name) + } else { + format!("Click here for the full {} patch", tool_name) + } + } - let tidy_summary = if test_params.tidy_review { - if test_params.force_lgtm { - "No concerns reported by clang-tidy. Great job! :tada:" - } else { - "Click here for the full clang-tidy patch" - } - } else { - "" - }; - let format_summary = if test_params.format_review { - if test_params.force_lgtm { - "No concerns reported by clang-format. Great job! :tada:" - } else { - "Click here for the full clang-format patch" - } - } else { - "" - }; + let tidy_summary = generate_tool_summary(test_params.tidy_review, test_params.force_lgtm, "clang-tidy"); + let format_summary = generate_tool_summary(test_params.format_review, test_params.force_lgtm, "clang-format");
262-280: LGTM! Good test coverage for individual tool configurationsThe new test cases effectively cover scenarios where only one tool (tidy or format) is enabled. Consider adding similar test cases for:
changed_lines_tidy_onlychanged_lines_format_onlyThis would provide complete coverage for both
LinesChangedOnlymodes.#[tokio::test] async fn changed_lines_tidy_only() { test_review(&TestParams { format_review: false, ..Default::default() }) .await; } #[tokio::test] async fn changed_lines_format_only() { test_review(&TestParams { tidy_review: false, ..Default::default() }) .await; }cpp-linter/src/clang_tools/clang_tidy.rs (1)
315-318: Consider using map_err for contextThe code correctly handles the new return type. Consider adding context to the error using
map_errfor better error messages.- file.tidy_advice = Some(parse_tidy_output( - &output.stdout, - &clang_params.database_json, - )?); + file.tidy_advice = Some(parse_tidy_output( + &output.stdout, + &clang_params.database_json, + ).map_err(|e| anyhow::anyhow!("Failed to parse clang-tidy output: {}", e))?);cpp-linter/src/rest_api/github/mod.rs (1)
402-405: Consider using realistic version numbers in testsWhile the current test setup works, using actual clang version formats (e.g., "15.0.7" or "16.0.0") would make the tests more realistic and could catch potential version parsing issues.
let clang_versions = ClangVersions { - format_version: Some("x.y.z".to_string()), - tidy_version: Some("x.y.z".to_string()), + format_version: Some("15.0.7".to_string()), + tidy_version: Some("16.0.0".to_string()), };cpp-linter/src/rest_api/github/specific_api.rs (1)
374-375: Fix indentation for consistencyThe condition's indentation should match the surrounding code style for better readability.
- } else if has_no_changes && review_comments.comments.is_empty() { - // if patches have no changes AND there are no comments about clang-tidy diagnostics + } else if has_no_changes && review_comments.comments.is_empty() { + // if patches have no changes AND there are no comments about clang-tidy diagnosticscpp-linter/src/clang_tools/mod.rs (2)
289-290: Avoid usingunwrap(); consider safer pattern matching withif let.Using
unwrap()onOptiontypes can panic if not carefully managed. Although you've ensured thattool_totalisSomebefore unwrapping, it's safer and more idiomatic to use pattern matching to handleOptionwithout risking panics.Here's a suggested change:
- let tool_total = self.tool_total[t as usize].unwrap(); + let tool_total = if let Some(total) = self.tool_total[t as usize] { + total + } else { + continue; + };This approach eliminates the need for
unwrap()and handles theNonecase explicitly.
392-394: Simplify initialization oftool_totalusingget_or_insert.You can streamline the initialization of
tool_totalby usingget_or_insert(0), which sets the value toSome(0)if it isNone, enhancing code readability.Here's a suggested change:
- if review_comments.tool_total[is_tidy_tool as usize].is_none() { - review_comments.tool_total[is_tidy_tool as usize] = Some(0); - } + review_comments.tool_total[is_tidy_tool as usize].get_or_insert(0);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
cpp-linter/src/clang_tools/clang_tidy.rs(3 hunks)cpp-linter/src/clang_tools/mod.rs(9 hunks)cpp-linter/src/common_fs/mod.rs(5 hunks)cpp-linter/src/rest_api/github/mod.rs(7 hunks)cpp-linter/src/rest_api/github/specific_api.rs(3 hunks)cpp-linter/src/rest_api/mod.rs(10 hunks)cpp-linter/src/run.rs(2 hunks)cpp-linter/tests/reviews.rs(4 hunks)cspell.config.yml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- cspell.config.yml
🔇 Additional comments (21)
cpp-linter/src/run.rs (1)
140-141: Verify feedback system integration.
The clang versions are correctly passed to the feedback system. Let's verify that the feedback system properly handles this information.
✅ Verification successful
The search results show that clang_versions is properly integrated throughout the feedback system:
- It's correctly passed through the trait interface in
rest_api/mod.rs - The GitHub-specific implementation in
rest_api/github/specific_api.rsuses it in the review summary - The versions are used in
clang_tools/mod.rsfor generating the summary message - The error handling with
?operator ensures proper propagation
Let me verify the actual summary generation to ensure the versions are properly formatted:
Let me try a different approach to check the summary implementation:
Integration with feedback system verified successfully
The clang versions are properly integrated in the feedback system. The implementation in clang_tools/mod.rs correctly uses the versions in the summary generation, formatting them as "Used clang-format " and "Used clang-tidy " in the review comments. The error handling with the ? operator ensures proper propagation of any errors that might occur during the feedback process.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the feedback system properly handles clang versions
# by checking the implementation of post_feedback in the RestApiClient trait
# and its implementations.
# Check the trait definition and implementations
rg -A 10 "fn post_feedback.*clang_versions" --type rust
# Check for any direct usage of clang_versions in the feedback formatting
rg "clang_versions" --type rust
Length of output: 2035
Script:
#!/bin/bash
# Check the implementation of the summarize method
ast-grep --pattern 'fn summarize(&self, clang_versions: &ClangVersions) -> String { $$$ }'
Length of output: 92
Script:
#!/bin/bash
# Check the implementation of the summarize method using ripgrep with context
rg -A 20 "fn summarize.*ClangVersions.*String" --type rust
Length of output: 1775
cpp-linter/src/common_fs/mod.rs (1)
Line range hint 313-343: LGTM! Test names are now more descriptive.
The test renames from numeric suffixes to descriptive names (none, diff, added) improve code readability and maintainability by clearly indicating each test's purpose.
cpp-linter/tests/reviews.rs (1)
209-209: LGTM! Well-documented tool configurations
The code clearly explains the reasoning behind each configuration setting through comments, making it easy to understand why specific checks and formatting rules are being applied when force_lgtm is true.
Also applies to: 226-234
cpp-linter/src/clang_tools/clang_tidy.rs (2)
221-224: LGTM!
The TidyAdvice construction is clean and consistent with the struct's design.
136-136: Verify handling of breaking change in function signature
The return type change from Result<Option<TidyAdvice>> to Result<TidyAdvice> is a breaking change that could affect error handling in calling code.
Let's verify all usages of this function to ensure they handle the new return type correctly:
✅ Verification successful
Breaking change in return type is properly handled
The change from Result<Option<TidyAdvice>> to Result<TidyAdvice> is correctly handled in the codebase. The only usage of parse_tidy_output is in a context where:
- The result is properly handled with the
?operator - The returned value is immediately wrapped in
Some()when assigned tofile.tidy_advice - Subsequent code correctly checks for the presence of
tidy_adviceusingif let Some(tidy_advice)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all calls to parse_tidy_output and verify they handle the new return type
# Expected: All callers should handle Result<TidyAdvice> instead of Result<Option<TidyAdvice>>
# Search for function calls
rg "parse_tidy_output\(" --type rust
Length of output: 189
Script:
#!/bin/bash
# Let's examine the context around the usage to verify error handling
rg "parse_tidy_output\(" --type rust -B 5 -A 5
Length of output: 1705
cpp-linter/src/rest_api/github/mod.rs (4)
23-23: LGTM: Import correctly placed
The ClangVersions import is properly organized with other project-specific imports.
244-250: LGTM: Consistent parameter passing
The clang_versions parameter is correctly passed to both make_comment calls with consistent parameter ordering.
Also applies to: 266-266
295-296: LGTM: Proper reference passing
The clang_versions parameter is correctly passed as a reference to post_review.
234-234: Verify implementation across the codebase
The addition of the clang_versions parameter is a breaking change. Let's verify that all implementations and callers have been updated.
✅ Verification successful
Let me analyze the trait definition to ensure all implementations match the signature.
Let me try a different approach to check the trait and implementations.
Let me try one final search with a simpler pattern to locate the trait definition.
Let me try one last search focusing on the specific file where the trait should be defined.
Based on the analysis of the code, I can now provide a final response:
Breaking change is properly handled across the codebase
The post_feedback method signature change with clang_versions: ClangVersions parameter is consistently implemented:
- The trait definition in
rest_api/mod.rsincludes the parameter - All implementations (GitHub client) include the parameter
- All call sites pass the
clang_versionsparameter correctly - Test code has been updated to use
ClangVersions::default()
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for implementations and calls to post_feedback
rg -A 2 "post_feedback.*\("
Length of output: 1474
Script:
#!/bin/bash
# Find the trait definition and implementations
ast-grep --pattern 'trait ResApiClient {
$$$
async fn post_feedback($$$) {
$$$
}
$$$
}'
ast-grep --pattern 'impl ResApiClient for $_ {
$$$
async fn post_feedback($$$) {
$$$
}
$$$
}'
Length of output: 209
Script:
#!/bin/bash
# Search for the trait definition and implementations with more context
rg -A 10 "trait ResApiClient"
rg -A 10 "impl ResApiClient"
Length of output: 62
Script:
#!/bin/bash
# Search for ResApiClient in source files
rg "ResApiClient" --type rust
Length of output: 147
Script:
#!/bin/bash
# Check the content of rest_api/mod.rs which should contain the trait definition
cat cpp-linter/src/rest_api/mod.rs
Length of output: 25228
cpp-linter/src/rest_api/github/specific_api.rs (4)
15-15: LGTM: Import addition is well-organized
The ClangVersions import is correctly placed within the existing clang_tools module imports.
308-308: LGTM: Parameter addition follows Rust conventions
The clang_versions parameter is correctly added as a reference type, following Rust's borrowing rules and naming conventions.
383-383: LGTM: Version information correctly integrated
The clang_versions parameter is properly passed to the summarize method, enabling version information to be included in the review body.
Line range hint 308-383: Verify all callers of post_review are updated
Let's ensure all callers of post_review have been updated to provide the new clang_versions parameter.
✅ Verification successful
All callers of post_review are correctly updated with the clang_versions parameter
The verification shows that there is only one caller of post_review in cpp-linter/src/rest_api/github/mod.rs and it correctly provides the clang_versions parameter. No further updates are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all calls to post_review to verify they provide clang_versions
rg -A 3 "post_review\("
Length of output: 652
cpp-linter/src/rest_api/mod.rs (4)
20-20: LGTM! Ownership model is well-designed.
The changes to import and integrate ClangVersions are consistent with Rust's ownership principles:
- Reference in
make_commentfor read-only access - Ownership transfer in
post_feedbackas the final consumer
Also applies to: 237-237, 289-289
337-339: LGTM! Format strings are well-structured.
The version information is integrated cleanly into the HTML structure while maintaining readability.
Also applies to: 368-368
470-470: LGTM! Test coverage is maintained.
The test modifications properly integrate the new ClangVersions parameter while maintaining existing test coverage and error cases.
Also applies to: 502-502
Line range hint 237-289: Verify consistent version usage across the codebase.
Let's ensure that the version information is consistently handled across different components.
✅ Verification successful
Let me gather more information to verify the version handling patterns more thoroughly.
Let me check one more aspect to understand the complete version handling structure.
Version handling is properly implemented and consistent across the codebase
The version handling in the code is well-structured and safe:
ClangVersionsstruct properly usesOption<String>for bothformat_versionandtidy_version- The unwrap calls in the review comment are safe as they are preceded by conditional checks
- Version information is consistently passed through the codebase using the
ClangVersionsstruct - Error handling for versions is properly implemented with Option types
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent version handling patterns across the codebase
# Check for unwrap usage on version fields
echo "Checking for potentially unsafe version unwrapping:"
rg "version.*unwrap" -t rust
# Check for consistent version parameter naming
echo "Checking version parameter naming consistency:"
ast-grep --pattern 'fn $func($$$, clang_versions: $_) {'
Length of output: 1376
Script:
#!/bin/bash
# Check for all version-related struct definitions and their usage
echo "Checking version-related struct definitions:"
ast-grep --pattern 'struct $name {
$$$
version: $_,
$$$
}'
echo -e "\nChecking ClangVersions struct usage:"
ast-grep --pattern 'struct ClangVersions {
$$$
}'
echo -e "\nChecking version-related function parameters:"
rg "fn.*version" -t rust
echo -e "\nChecking error handling for versions:"
rg "version.*None" -t rust
Length of output: 1053
Script:
#!/bin/bash
# Check the ClangVersions struct definition and its usage patterns
echo "Checking ClangVersions struct definition:"
rg "ClangVersions" -A 10 -t rust
echo -e "\nChecking Option handling for versions:"
rg "Option.*version" -t rust
echo -e "\nChecking version-related error handling:"
rg "Error.*version" -t rust
Length of output: 12453
cpp-linter/src/clang_tools/mod.rs (4)
16-16: Importing regex::Regex is appropriate.
The addition of use regex::Regex; is necessary for regex operations introduced in the code changes below.
139-147: Introducing ClangVersions struct enhances code organization.
Defining the ClangVersions struct encapsulates the version information of clang-format and clang-tidy, improving code maintainability and clarity.
170-192: Capturing tool versions enhances functionality.
Logging the versions of clang-tidy and clang-format improves transparency and assists in debugging. The implementation correctly updates clang_versions and handles cases where tools are not used.
452-454: Properly updating tool_total using unwrap_or_default().
Using unwrap_or_default() ensures that tool_total is incremented correctly even if it was None, defaulting to zero. This is an effective way to handle optional values while avoiding panics.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
cpp-linter/src/clang_tools/mod.rs (1)
149-159: Consider adding version string validation.While the regex pattern is robust, consider validating the extracted version string to ensure it matches expected format (e.g., major.minor.patch). This would help catch unexpected version string formats early.
fn capture_clang_version(clang_tool: &PathBuf) -> Result<String> { let output = Command::new(clang_tool).arg("--version").output()?; let stdout = String::from_utf8_lossy(&output.stdout); let version_pattern = Regex::new(r"(?i)version\s*([\d.]+)").unwrap(); let captures = version_pattern.captures(&stdout).ok_or(anyhow!( "Failed to find version number in `{} --version` output", clang_tool.to_string_lossy() ))?; - Ok(captures.get(1).unwrap().as_str().to_string()) + let version = captures.get(1).unwrap().as_str(); + // Validate version string format + if !version.split('.').all(|part| part.parse::<u32>().is_ok()) { + return Err(anyhow!( + "Invalid version format in `{} --version` output: {}", + clang_tool.to_string_lossy(), + version + )); + } + Ok(version.to_string())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
cpp-linter/src/clang_tools/mod.rs(9 hunks)
🔇 Additional comments (3)
cpp-linter/src/clang_tools/mod.rs (3)
139-147: LGTM! Well-designed version tracking struct.
The ClangVersions struct is well-documented and appropriately uses Option<String> for version fields, allowing for cases where tools aren't used.
170-182: LGTM! Well-implemented version capture and storage.
The changes effectively capture and store tool versions while maintaining good error handling and appropriate debug logging.
Also applies to: 185-192, 224-224
246-246: LGTM! Type-safe improvements to review comments handling.
The changes improve type safety by using Option<u32> for tool totals and properly integrate version information into the summary output. The unwrap calls are safe due to proper null checks.
Also applies to: 259-278, 288-290, 392-394, 452-454
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
cpp-linter/tests/reviews.rs (2)
63-72: LGTM! Consider adding documentation.The function is well-structured and effectively handles the generation of tool summaries. Consider adding documentation comments to describe the parameters and return value.
Add documentation like this:
+/// Generates a summary message for a specific clang tool based on review settings +/// +/// # Arguments +/// * `review_enabled` - Whether the review for this tool is enabled +/// * `force_lgtm` - Whether LGTM mode is forced +/// * `tool_name` - Name of the clang tool (e.g., "clang-tidy", "clang-format") +/// +/// # Returns +/// A string containing the appropriate summary message or an empty string if review is disabled fn generate_tool_summary(review_enabled: bool, force_lgtm: bool, tool_name: &str) -> String {
212-212: LGTM! Consider extracting magic strings as constants.The configuration logic is well-structured with clear comments explaining the purpose. Consider extracting the file patterns and check configurations as constants for better maintainability.
Extract constants like this:
+const CPP_FILE_PATTERNS: &str = "|**/*.cpp|**/*.h"; +const SAFE_TIDY_CHECKS: &str = "-*,bugprone-infinite-loop"; +const DISABLED_FORMAT_STYLE: &str = "{DisableFormat: true}"; + let mut tool_ignore = "**/*.c".to_string(); if test_params.force_lgtm { - tool_ignore.push_str("|**/*.cpp|**/*.h"); + tool_ignore.push_str(CPP_FILE_PATTERNS); } // ... if test_params.force_lgtm { if test_params.tidy_review { - args.push("--tidy-checks=-*,bugprone-infinite-loop".to_string()); + args.push(format!("--tidy-checks={}", SAFE_TIDY_CHECKS)); } if test_params.format_review { - args.push("--style={DisableFormat: true}".to_string()); + args.push(format!("--style={}", DISABLED_FORMAT_STYLE)); } }Also applies to: 229-238
cpp-linter/src/clang_tools/mod.rs (2)
170-192: Enhance version logging consistency.The version logging implementation is good, but consider making the log format more consistent and adding error context.
- log::debug!( - "{} --version: v{version_found}", - &exe_path.to_string_lossy() - ); + log::debug!( + "Found {} version {} at {}", + if t == 0 { "clang-format" } else { "clang-tidy" }, + version_found, + &exe_path.to_string_lossy() + );
259-280: Improve version information formatting in summary.The version information integration looks good, but consider making the format more consistent with markdown standards.
- body.push_str(format!("### Used {tool_name} {ver_str}\n").as_str()); + body.push_str(format!("### {tool_name} v{ver_str}\n").as_str());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
cpp-linter/src/clang_tools/mod.rs(9 hunks)cpp-linter/src/common_fs/mod.rs(5 hunks)cpp-linter/tests/reviews.rs(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp-linter/src/common_fs/mod.rs
🔇 Additional comments (4)
cpp-linter/tests/reviews.rs (2)
175-184: LGTM! Clean refactoring of summary generation.
The integration of the new generate_tool_summary function improves code maintainability while preserving the existing functionality.
265-283: LGTM! Good test coverage for individual tool configurations.
The new test cases effectively verify the behavior when only clang-tidy or clang-format is enabled, following the established testing patterns.
cpp-linter/src/clang_tools/mod.rs (2)
139-147: LGTM! Well-structured version tracking.
The ClangVersions struct is well-designed with appropriate use of Option for version fields.
450-452: LGTM! Proper handling of tool totals.
The initialization and update of tool totals is well-implemented with proper handling of Option values.
This affects thread-comments, step-summary, and PR review summary.
I also added tests to ensure PR review summary only shows relevant information about the tools that were actually used.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores