Skip to content

Commit c4bf3d0

Browse files
authored
fix: clang-tidy diagnostic comments in PR review (#77)
* prevent tidy diagnostics outside of PR's diff * fix markdown format of diagnostic comment
1 parent 205bd2d commit c4bf3d0

File tree

3 files changed

+43
-5
lines changed

3 files changed

+43
-5
lines changed

cpp-linter/src/clang_tools/mod.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -180,7 +180,7 @@ pub async fn capture_clang_tools_output(
180180
);
181181
clang_versions.tidy_version = Some(version_found);
182182
clang_params.clang_tidy_command = Some(exe_path);
183-
};
183+
}
184184
if !clang_params.style.is_empty() {
185185
let exe_path = get_clang_tool_exe("clang-format", version)?;
186186
let version_found = capture_clang_version(&exe_path)?;
@@ -190,7 +190,7 @@ pub async fn capture_clang_tools_output(
190190
);
191191
clang_versions.format_version = Some(version_found);
192192
clang_params.clang_format_command = Some(exe_path);
193-
};
193+
}
194194

195195
// parse database (if provided) to match filenames when parsing clang-tidy's stdout
196196
if let Some(db_path) = &clang_params.database {

cpp-linter/src/common_fs/mod.rs

+24-3
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,20 @@ impl FileObj {
119119
None
120120
}
121121

122+
/// Similar to [`FileObj::is_hunk_in_diff()`] but looks for a single line instead of
123+
/// an entire [`DiffHunk`].
124+
///
125+
/// This is a private function because it is only used in
126+
/// [`FileObj::make_suggestions_from_patch()`].
127+
fn is_line_in_diff(&self, line: &u32) -> bool {
128+
for range in &self.diff_chunks {
129+
if range.contains(line) {
130+
return true;
131+
}
132+
}
133+
false
134+
}
135+
122136
/// Create a list of [`Suggestion`](struct@crate::clang_tools::Suggestion) from a
123137
/// generated [`Patch`](struct@git2::Patch) and store them in the given
124138
/// [`ReviewComments`](struct@crate::clang_tools::ReviewComments).
@@ -161,10 +175,10 @@ impl FileObj {
161175
// Count of clang-tidy diagnostics that had no fixes applied
162176
let mut total = 0;
163177
for note in &advice.notes {
164-
if note.fixed_lines.is_empty() {
178+
if note.fixed_lines.is_empty() && self.is_line_in_diff(&note.line) {
165179
// notification had no suggestion applied in `patched`
166180
let mut suggestion = format!(
167-
"### clang-tidy diagnostic\n**{file_name}:{}:{}** {}: [{}]\n> {}",
181+
"### clang-tidy diagnostic\n**{file_name}:{}:{}** {}: [{}]\n\n> {}\n",
168182
&note.line,
169183
&note.cols,
170184
&note.severity,
@@ -173,7 +187,8 @@ impl FileObj {
173187
);
174188
if !note.suggestion.is_empty() {
175189
suggestion.push_str(
176-
format!("```{file_ext}\n{}```", &note.suggestion.join("\n")).as_str(),
190+
format!("\n```{file_ext}\n{}\n```\n", &note.suggestion.join("\n"))
191+
.as_str(),
177192
);
178193
}
179194
total += 1;
@@ -342,4 +357,10 @@ mod test {
342357
let ranges = file_obj.get_ranges(&LinesChangedOnly::On);
343358
assert_eq!(ranges, vec![4..=5, 9..=9]);
344359
}
360+
361+
#[test]
362+
fn line_not_in_diff() {
363+
let file_obj = FileObj::new(PathBuf::from("tests/demo/demo.cpp"));
364+
assert!(!file_obj.is_line_in_diff(&42));
365+
}
345366
}

cpp-linter/src/run.rs

+17
Original file line numberDiff line numberDiff line change
@@ -221,4 +221,21 @@ mod test {
221221
.await;
222222
assert!(result.is_err());
223223
}
224+
225+
// Verifies that the system gracefully handles cases where all analysis is disabled.
226+
// This ensures no diagnostic comments are generated when analysis is explicitly skipped.
227+
#[tokio::test]
228+
async fn no_analysis() {
229+
env::remove_var("GITHUB_OUTPUT"); // avoid writing to GH_OUT in parallel-running tests
230+
let result = run_main(vec![
231+
"cpp-linter".to_string(),
232+
"-l".to_string(),
233+
"false".to_string(),
234+
"--style".to_string(),
235+
String::new(),
236+
"--tidy-checks=-*".to_string(),
237+
])
238+
.await;
239+
assert!(result.is_ok());
240+
}
224241
}

0 commit comments

Comments
 (0)