Skip to content
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

Commit 391ffa9

Browse files
committedNov 23, 2024·
final review
1 parent ddccd00 commit 391ffa9

File tree

3 files changed

+28
-26
lines changed

3 files changed

+28
-26
lines changed
 

‎cpp-linter/src/clang_tools/mod.rs

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -146,7 +146,7 @@ pub struct ClangVersions {
146146
pub tidy_version: Option<String>,
147147
}
148148

149-
/// Run `clang-tool --version`, the extract and return the version number.
149+
/// Run `clang-tool --version`, then extract and return the version number.
150150
fn capture_clang_version(clang_tool: &PathBuf) -> Result<String> {
151151
let output = Command::new(clang_tool).arg("--version").output()?;
152152
let stdout = String::from_utf8_lossy(&output.stdout);
@@ -266,10 +266,12 @@ impl ReviewComments {
266266
("clang-tidy", clang_versions.tidy_version.as_ref())
267267
};
268268

269-
if self.tool_total[t as usize].is_none() {
269+
let tool_total = if let Some(total) = self.tool_total[t as usize] {
270+
total
271+
} else {
270272
// review was not requested from this tool or the tool was not used at all
271273
continue;
272-
}
274+
};
273275

274276
// If the tool's version is unknown, then we don't need to output this line.
275277
// NOTE: If the tool was invoked at all, then the tool's version shall be known.
@@ -285,8 +287,6 @@ impl ReviewComments {
285287
}
286288
}
287289

288-
// at this point tool_total is Some() value; unwrap() is safe here
289-
let tool_total = self.tool_total[t as usize].unwrap();
290290
if total != tool_total {
291291
body.push_str(
292292
format!(
@@ -389,9 +389,7 @@ pub trait MakeSuggestions {
389389
.with_context(|| format!("Failed to convert patch to string: {file_name}"))?
390390
.as_str(),
391391
);
392-
if review_comments.tool_total[is_tidy_tool as usize].is_none() {
393-
review_comments.tool_total[is_tidy_tool as usize] = Some(0);
394-
}
392+
review_comments.tool_total[is_tidy_tool as usize].get_or_insert(0);
395393
if summary_only {
396394
return Ok(());
397395
}

‎cpp-linter/src/common_fs/mod.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,7 @@ impl FileObj {
158158
.unwrap_or_default()
159159
.to_str()
160160
.unwrap_or_default();
161+
// Count of clang-tidy diagnostics that had no fixes applied
161162
let mut total = 0;
162163
for note in &advice.notes {
163164
if note.fixed_lines.is_empty() {

‎cpp-linter/tests/reviews.rs

Lines changed: 21 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,17 @@ impl Default for TestParams {
6060
}
6161
}
6262

63+
fn generate_tool_summary(review_enabled: bool, force_lgtm: bool, tool_name: &str) -> String {
64+
if !review_enabled {
65+
return String::new();
66+
}
67+
if force_lgtm {
68+
format!("No concerns reported by {}. Great job! :tada:", tool_name)
69+
} else {
70+
format!("Click here for the full {} patch", tool_name)
71+
}
72+
}
73+
6374
async fn setup(lib_root: &Path, test_params: &TestParams) {
6475
env::remove_var("GITHUB_OUTPUT"); // avoid writing to GH_OUT in parallel-running tests
6576
env::set_var("GITHUB_EVENT_NAME", "pull_request");
@@ -161,24 +172,16 @@ async fn setup(lib_root: &Path, test_params: &TestParams) {
161172
} else {
162173
"REQUEST_CHANGES"
163174
};
164-
let tidy_summary = if test_params.tidy_review {
165-
if test_params.force_lgtm {
166-
"No concerns reported by clang-tidy. Great job! :tada:"
167-
} else {
168-
"Click here for the full clang-tidy patch"
169-
}
170-
} else {
171-
""
172-
};
173-
let format_summary = if test_params.format_review {
174-
if test_params.force_lgtm {
175-
"No concerns reported by clang-format. Great job! :tada:"
176-
} else {
177-
"Click here for the full clang-format patch"
178-
}
179-
} else {
180-
""
181-
};
175+
let tidy_summary = generate_tool_summary(
176+
test_params.tidy_review,
177+
test_params.force_lgtm,
178+
"clang-tidy",
179+
);
180+
let format_summary = generate_tool_summary(
181+
test_params.format_review,
182+
test_params.force_lgtm,
183+
"clang-format",
184+
);
182185
let review_summary = format!(
183186
"{}## Cpp-linter Review.*{format_summary}.*{tidy_summary}.*{}",
184187
regex::escape(format!("{}", COMMENT_MARKER.escape_default()).as_str()),

0 commit comments

Comments
 (0)
Please sign in to comment.