Skip to content

Commit 0d792c2

Browse files
2bndy52bndy5
committed
assessing review summary
1 parent a5c48c4 commit 0d792c2

File tree

5 files changed

+86
-63
lines changed

5 files changed

+86
-63
lines changed

‎cpp-linter-lib/src/clang_tools/clang_tidy.rs

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -96,8 +96,10 @@ impl MakeSuggestions for TidyAdvice {
9696
fn get_suggestion_help(&self, start_line: u32, end_line: u32) -> String {
9797
let mut diagnostics = vec![];
9898
for note in &self.notes {
99-
if (start_line..end_line).contains(&note.line) {
100-
diagnostics.push(format!("- {}\n", note.diagnostic_link()));
99+
for fixed_line in &note.fixed_lines {
100+
if (start_line..=end_line).contains(fixed_line) {
101+
diagnostics.push(format!("- {}\n", note.diagnostic_link()));
102+
}
101103
}
102104
}
103105
format!(

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

Lines changed: 9 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -235,15 +235,6 @@ pub struct ReviewComments {
235235
}
236236

237237
impl ReviewComments {
238-
pub fn is_hunk_in_suggestions(&self, start_line: u32, end_line: u32) -> bool {
239-
for suggestion in &self.comments {
240-
if start_line >= suggestion.line_start && suggestion.line_end >= end_line {
241-
return true;
242-
}
243-
}
244-
false
245-
}
246-
247238
pub fn summarize(&self) -> String {
248239
let mut body = format!("{COMMENT_MARKER}## Cpp-linter Review\n");
249240
let comments_len = self.comments.len() as u32;
@@ -260,30 +251,26 @@ impl ReviewComments {
260251
}
261252
}
262253

263-
let is_tidy_tool = tool_name == "clang-tidy";
264-
if total != self.tool_total[is_tidy_tool as usize] {
254+
if total != self.tool_total[t as usize] {
265255
body.push_str(
266256
format!(
267-
"Only {} out of {} {tool_name} concerns fit within this pull request's diff.\n",
268-
self.tool_total[is_tidy_tool as usize], total
257+
"\nOnly {} out of {} {tool_name} concerns fit within this pull request's diff.\n",
258+
self.tool_total[t as usize], total
269259
)
270260
.as_str(),
271261
);
272262
}
273263
if total > 0 {
274264
body.push_str(
275265
format!(
276-
"\n<details><summary>Click here for the {tool_name} full patch</summary>\n\n```diff\n{}```\n\n</details>",
277-
self.full_patch[is_tidy_tool as usize]
266+
"\n<details><summary>Click here for the full {tool_name} patch</summary>\n\n```diff\n{}```\n\n</details>\n",
267+
self.full_patch[t as usize]
278268
).as_str()
279269
);
280270
} else {
281271
body.push_str(
282-
format!(
283-
"\n\nNo concerns reported by {}. Great job! :tada:",
284-
tool_name
285-
)
286-
.as_str(),
272+
format!("\nNo concerns reported by {}. Great job! :tada:", tool_name)
273+
.as_str(),
287274
)
288275
}
289276
}
@@ -298,6 +285,7 @@ impl ReviewComments {
298285
&& s.line_end == comment.line_end
299286
&& s.line_start == comment.line_start
300287
{
288+
s.suggestion.push('\n');
301289
s.suggestion.push_str(comment.suggestion.as_str());
302290
return true;
303291
}
@@ -368,16 +356,12 @@ pub trait MakeSuggestions {
368356
);
369357
for hunk_id in 0..hunks_total {
370358
let (hunk, line_count) = patch.hunk(hunk_id).expect("Failed to get hunk from patch");
359+
hunks_in_patch += 1;
371360
let hunk_range = file_obj.is_hunk_in_diff(&hunk);
372361
if hunk_range.is_none() {
373362
continue;
374363
}
375364
let (start_line, end_line) = hunk_range.unwrap();
376-
if review_comments.is_hunk_in_suggestions(start_line, end_line) {
377-
// Here we must assume that clang-tidy suggestions already apply clang-format suggestions
378-
continue;
379-
}
380-
hunks_in_patch += 1;
381365
let mut suggestion = String::new();
382366
let suggestion_help = if !summary_only {
383367
Some(self.get_suggestion_help(start_line, end_line))

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

Lines changed: 17 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -130,37 +130,38 @@ impl FileObj {
130130
) {
131131
let original_content =
132132
fs::read(&self.name).expect("Failed to read original contents of file");
133-
if let Some(advice) = &self.tidy_advice {
133+
134+
if let Some(advice) = &self.format_advice {
134135
if let Some(patched) = &advice.patched {
135136
let mut patch = make_patch(&self.name, patched, &original_content);
136137
advice.get_suggestions(review_comments, self, &mut patch, summary_only);
137138
}
138139
}
139140

140-
if let Some(advice) = &self.format_advice {
141+
if let Some(advice) = &self.tidy_advice {
141142
if let Some(patched) = &advice.patched {
142143
let mut patch = make_patch(&self.name, patched, &original_content);
143144
advice.get_suggestions(review_comments, self, &mut patch, summary_only);
144145
}
145-
}
146146

147-
let file_name = self
148-
.name
149-
.to_str()
150-
.expect("Failed to convert file extension to string")
151-
.replace("\\", "/");
152-
// now check for clang-tidy warnings with no fixes applied
153-
if let Some(advice) = &self.tidy_advice {
147+
// now check for clang-tidy warnings with no fixes applied
148+
let file_name = self
149+
.name
150+
.to_str()
151+
.expect("Failed to convert file extension to string")
152+
.replace("\\", "/");
153+
let file_ext = self
154+
.name
155+
.extension()
156+
.unwrap_or_default()
157+
.to_str()
158+
.expect("Failed to convert file extension to string");
154159
for note in &advice.notes {
155160
if note.fixed_lines.is_empty() {
156161
// notification had no suggestion applied in `patched`
157162
let mut suggestion = format!(
158163
"### clang-tidy diagnostic\n**{}:{}:{}** {}: [{}]\n> {}",
159-
&self
160-
.name
161-
.to_str()
162-
.expect("Failed to convert file name to string")
163-
.replace("\\", "/"),
164+
file_name,
164165
&note.line,
165166
&note.cols,
166167
&note.severity,
@@ -169,18 +170,7 @@ impl FileObj {
169170
);
170171
if !note.suggestion.is_empty() {
171172
suggestion.push_str(
172-
format!(
173-
"```{}\n{}```",
174-
&self
175-
.name
176-
.extension()
177-
.unwrap_or_default()
178-
.to_str()
179-
.expect("Failed to convert file extension to string")
180-
.trim_start_matches("."),
181-
&note.suggestion.join("\n")
182-
)
183-
.as_str(),
173+
format!("```{}\n{}```", file_ext, &note.suggestion.join("\n")).as_str(),
184174
);
185175
}
186176
review_comments.tool_total[1] += 1;

‎cpp-linter-lib/src/rest_api/github_api.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -703,6 +703,7 @@ struct ReviewDiffComment {
703703
pub body: String,
704704
pub line: i64,
705705
pub start_line: Option<i64>,
706+
pub path: String,
706707
}
707708

708709
impl From<Suggestion> for ReviewDiffComment {
@@ -715,6 +716,7 @@ impl From<Suggestion> for ReviewDiffComment {
715716
} else {
716717
None
717718
},
719+
path: value.path,
718720
}
719721
}
720722
}

‎cpp-linter-lib/tests/reviews.rs

Lines changed: 54 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -29,19 +29,23 @@ async fn setup(
2929
format_review: bool,
3030
passive_reviews: bool,
3131
no_lgtm: bool,
32+
summary_only: bool,
3233
) {
3334
env::set_var("GITHUB_EVENT_NAME", "pull_request");
3435
env::set_var("GITHUB_REPOSITORY", REPO);
3536
env::set_var("GITHUB_SHA", SHA);
3637
env::set_var("GITHUB_TOKEN", TOKEN);
3738
env::set_var("CI", "true");
39+
if summary_only {
40+
env::set_var("CPP_LINTER_PR_SUMMARY_ONLY", "true");
41+
}
3842
let mut event_payload_path = NamedTempFile::new().unwrap();
3943
event_payload_path
4044
.write_all(EVENT_PAYLOAD.as_bytes())
4145
.expect("Failed to create mock event payload.");
4246
env::set_var("GITHUB_EVENT_PATH", event_payload_path.path());
4347
let reset_timestamp = (Utc::now().timestamp() + 60).to_string();
44-
let is_lgtm = no_lgtm || ((false, false) == (tidy_review, format_review));
48+
let is_lgtm = !no_lgtm || ((false, false) == (tidy_review, format_review));
4549
let asset_path = format!("{}/{MOCK_ASSETS_PATH}", lib_root.to_str().unwrap());
4650

4751
let mut server = mock_server().await;
@@ -84,6 +88,7 @@ async fn setup(
8488
.with_header(REMAINING_RATE_LIMIT_HEADER, "50")
8589
.with_header(RESET_RATE_LIMIT_HEADER, reset_timestamp.as_str())
8690
.create();
91+
8792
let review_reaction = if passive_reviews {
8893
"COMMENT"
8994
} else if is_lgtm {
@@ -102,13 +107,19 @@ async fn setup(
102107
.with_header(RESET_RATE_LIMIT_HEADER, reset_timestamp.as_str())
103108
.create();
104109

110+
let mut tool_ignore = "**.c".to_string();
111+
if !no_lgtm || (false, true) == (tidy_review, format_review) {
112+
// force a LGTM condition by skipping analysis on all files
113+
tool_ignore.push('|');
114+
tool_ignore.push_str("src");
115+
}
105116
let mut args = vec![
106117
"cpp-linter".to_string(),
107118
"-v=debug".to_string(),
108119
format!("-V={}", env::var("CLANG_VERSION").unwrap_or("".to_string())),
109120
format!("-l={lines_changed_only}"),
110-
"--ignore-tidy=**.c".to_string(),
111-
"--ignore-format=**.c".to_string(),
121+
format!("--ignore-tidy={}", tool_ignore),
122+
format!("--ignore-format={}", tool_ignore),
112123
// "--tidy-checks=".to_string(), // use .clang-tidy file
113124
"--style=file".to_string(), // use .clang-format file
114125
format!("--tidy-review={tidy_review}"),
@@ -131,6 +142,7 @@ async fn test_review(
131142
format_review: bool,
132143
passive_reviews: bool,
133144
no_lgtm: bool,
145+
summary_only: bool,
134146
) {
135147
let tmp_dir = create_test_space();
136148
let lib_root = env::current_dir().unwrap();
@@ -142,6 +154,7 @@ async fn test_review(
142154
format_review,
143155
passive_reviews,
144156
no_lgtm,
157+
summary_only,
145158
)
146159
.await;
147160
env::set_current_dir(lib_root.as_path()).unwrap();
@@ -155,7 +168,8 @@ async fn all_lines() {
155168
true, // tidy-review
156169
true, // format-review
157170
false, // passive-reviews
158-
false, // no_lgtm
171+
true, // no_lgtm
172+
false, // summary_only
159173
)
160174
.await;
161175
}
@@ -167,7 +181,8 @@ async fn changed_lines() {
167181
true, // tidy-review
168182
true, // format-review
169183
false, // passive-reviews
170-
false, // no_lgtm
184+
true, // no_lgtm
185+
false, // summary_only
171186
)
172187
.await;
173188
}
@@ -179,7 +194,8 @@ async fn all_lines_passive() {
179194
true, // tidy-review
180195
true, // format-review
181196
true, // passive-reviews
182-
false, // no_lgtm
197+
true, // no_lgtm
198+
false, // summary_only
183199
)
184200
.await;
185201
}
@@ -191,31 +207,60 @@ async fn changed_lines_passive() {
191207
true, // tidy-review
192208
true, // format-review
193209
true, // passive-reviews
194-
false, // no_lgtm
210+
true, // no_lgtm
211+
false, // summary_only
195212
)
196213
.await;
197214
}
198215

199216
#[tokio::test]
200-
async fn no_lgtm() {
217+
async fn summary_only() {
201218
test_review(
202219
"false", // lines_changed_only
203220
true, // tidy-review
204221
true, // format-review
205222
false, // passive-reviews
206223
true, // no_lgtm
224+
true, // summary_only
225+
)
226+
.await;
227+
}
228+
229+
#[tokio::test]
230+
async fn lgtm() {
231+
test_review(
232+
"false", // lines_changed_only
233+
true, // tidy-review
234+
true, // format-review
235+
false, // passive-reviews
236+
false, // no_lgtm
237+
false, // summary_only
207238
)
208239
.await;
209240
}
210241

211242
#[tokio::test]
212-
async fn no_lgtm_passive() {
243+
async fn lgtm_passive() {
213244
test_review(
214245
"false", // lines_changed_only
215246
true, // tidy-review
216247
true, // format-review
217248
true, // passive-reviews
249+
false, // no_lgtm
250+
false, // summary_only
251+
)
252+
.await;
253+
}
254+
255+
#[tokio::test]
256+
async fn no_lgtm() {
257+
test_review(
258+
"false", // lines_changed_only
259+
false, // tidy-review
260+
true, // format-review
261+
false, // passive-reviews
218262
true, // no_lgtm
263+
false, // summary_only
219264
)
220265
.await;
221266
}

0 commit comments

Comments
 (0)