encapsulate user inputs to post_feedback()

2bndy5 · 2bndy5 · commit b31be6ebec85 · 2024-01-17T17:06:14.000-08:00
also encapsulate `TidyNotification`s in a `TidyAdvice` struct
diff --git a/cpp-linter-lib/src/clang_tools/clang_tidy.rs b/cpp-linter-lib/src/clang_tools/clang_tidy.rs
@@ -68,14 +68,34 @@ pub struct TidyNotification {
     pub suggestion: Vec<String>,
 }
 
+impl TidyNotification {
+    pub fn diagnostic_link(&self) -> String {
+        let ret_val = if let Some((category, name)) = self.diagnostic.split_once('-') {
+            format!(
+                "[{}](https://clang.llvm.org/extra/clang-tidy/checks/{category}/{name}).html",
+                self.diagnostic
+            )
+        } else {
+            self.diagnostic.clone()
+        };
+        ret_val
+    }
+}
+
+/// A struct to hold notification from clang-tidy about a single file
+pub struct TidyAdvice {
+    /// A list of notifications parsed from clang-tidy stdout.
+    pub notes: Vec<TidyNotification>,
+}
+
 /// Parses clang-tidy stdout.
 ///
 /// Here it helps to have the JSON database deserialized for normalizing paths present
 /// in the notifications.
 fn parse_tidy_output(
     tidy_stdout: &[u8],
     database_json: &Option<CompilationDatabase>,
-) -> Vec<TidyNotification> {
+) -> TidyAdvice {
     let note_header = Regex::new(r"^(.+):(\d+):(\d+):\s(\w+):(.*)\[([a-zA-Z\d\-\.]+)\]$").unwrap();
     let mut notification = None;
     let mut result = Vec::new();
@@ -140,7 +160,7 @@ fn parse_tidy_output(
     if let Some(note) = notification {
         result.push(note);
     }
-    result
+    TidyAdvice { notes: result }
 }
 
 /// Run clang-tidy, then parse and return it's output.
@@ -152,7 +172,7 @@ pub fn run_clang_tidy(
     database: &Option<PathBuf>,
     extra_args: &Option<Vec<&str>>,
     database_json: &Option<CompilationDatabase>,
-) -> Vec<TidyNotification> {
+) -> TidyAdvice {
     if !checks.is_empty() {
         cmd.args(["-checks", checks]);
     }
@@ -263,6 +283,6 @@ mod test {
             vec!["--extra-arg", "\"-std=c++17\"", "--extra-arg", "\"-Wall\""],
             args
         );
-        assert!(!tidy_advice.is_empty());
+        assert!(!tidy_advice.notes.is_empty());
     }
 }
diff --git a/cpp-linter-lib/src/clang_tools/mod.rs b/cpp-linter-lib/src/clang_tools/mod.rs
@@ -14,7 +14,7 @@ use crate::logger::{end_log_group, start_log_group};
 pub mod clang_format;
 use clang_format::{run_clang_format, FormatAdvice};
 pub mod clang_tidy;
-use clang_tidy::{run_clang_tidy, CompilationDatabase, TidyNotification};
+use clang_tidy::{run_clang_tidy, CompilationDatabase, TidyAdvice};
 
 /// Fetch the path to a clang tool by `name` (ie `"clang-tidy"` or `"clang-format"`) and
 /// `version`.
@@ -69,7 +69,7 @@ pub fn get_clang_tool_exe(name: &str, version: &str) -> Result<PathBuf, &'static
 /// Runs clang-tidy and/or clang-format and returns the parsed output from each.
 ///
 /// The returned list of [`FormatAdvice`] is parallel to the `files` list passed in
-/// here. The returned 2D list of [`TidyNotification`] is also parallel on the first
+/// here. The returned 2D list of [`TidyAdvice`] is also parallel on the first
 /// dimension. The second dimension is a list of notes specific to a translation unit
 /// (each element of `files`).
 ///
@@ -83,7 +83,7 @@ pub fn capture_clang_tools_output(
     lines_changed_only: u8,
     database: Option<PathBuf>,
     extra_args: Option<Vec<&str>>,
-) -> (Vec<FormatAdvice>, Vec<Vec<TidyNotification>>) {
+) -> (Vec<FormatAdvice>, Vec<TidyAdvice>) {
     // find the executable paths for clang-tidy and/or clang-format and show version
     // info as debugging output.
     let clang_tidy_command = if tidy_checks != "-*" {
@@ -126,9 +126,8 @@ pub fn capture_clang_tools_output(
     };
 
     // iterate over the discovered files and run the clang tools
-    let mut all_format_advice: Vec<clang_format::FormatAdvice> = Vec::with_capacity(files.len());
-    let mut all_tidy_advice: Vec<Vec<clang_tidy::TidyNotification>> =
-        Vec::with_capacity(files.len());
+    let mut all_format_advice: Vec<FormatAdvice> = Vec::with_capacity(files.len());
+    let mut all_tidy_advice: Vec<TidyAdvice> = Vec::with_capacity(files.len());
     for file in files {
         start_log_group(format!("Analyzing {}", file.name.to_string_lossy()));
         if let Some(tidy_cmd) = &clang_tidy_command {
diff --git a/cpp-linter-lib/src/rest_api/github_api.rs b/cpp-linter-lib/src/rest_api/github_api.rs
@@ -13,11 +13,11 @@ use serde::Deserialize;
 use serde_json;
 
 // project specific modules/crates
-use crate::clang_tools::{clang_format::FormatAdvice, clang_tidy::TidyNotification};
+use crate::clang_tools::{clang_format::FormatAdvice, clang_tidy::TidyAdvice};
 use crate::common_fs::FileObj;
 use crate::git::{get_diff, open_repo, parse_diff, parse_diff_from_buf};
 
-use super::RestApiClient;
+use super::{FeedbackInput, RestApiClient, COMMENT_MARKER};
 
 static USER_AGENT: &str =
     "Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:120.0) Gecko/20100101 Firefox/120.0";
@@ -186,16 +186,12 @@ impl RestApiClient for GithubApiClient {
         &self,
         files: &[FileObj],
         format_advice: &[FormatAdvice],
-        tidy_advice: &[Vec<TidyNotification>],
-        thread_comments: &str,
-        no_lgtm: bool,
-        step_summary: bool,
-        file_annotations: bool,
-        style: &str,
+        tidy_advice: &[TidyAdvice],
+        user_inputs: FeedbackInput,
     ) {
         let (comment, format_checks_failed, tidy_checks_failed) =
             self.make_comment(files, format_advice, tidy_advice);
-        if thread_comments != "false" {
+        if user_inputs.thread_comments.as_str() != "false" {
             // post thread comment for PR or push event
             if let Some(repo) = &self.repo {
                 let is_pr = self.event_name == "pull_request";
@@ -222,15 +218,13 @@ impl RestApiClient for GithubApiClient {
                     } else {
                         json["commit"]["comment_count"].as_u64().unwrap()
                     };
-                    let user_id: u64 = 41898282;
                     self.update_comment(
                         &format!("{}/comments", &comments_url),
                         &comment,
                         count,
-                        user_id,
-                        no_lgtm,
+                        user_inputs.no_lgtm,
                         format_checks_failed + tidy_checks_failed == 0,
-                        thread_comments == "update",
+                        user_inputs.thread_comments.as_str() == "update",
                     );
                 } else {
                     let error = request.unwrap_err();
@@ -245,10 +239,15 @@ impl RestApiClient for GithubApiClient {
                 }
             }
         }
-        if file_annotations {
-            self.post_annotations(files, format_advice, tidy_advice, style);
+        if user_inputs.file_annotations {
+            self.post_annotations(
+                files,
+                format_advice,
+                tidy_advice,
+                user_inputs.style.as_str(),
+            );
         }
-        if step_summary {
+        if user_inputs.step_summary {
             self.post_step_summary(&comment);
         }
         self.set_exit_code(
@@ -276,7 +275,7 @@ impl GithubApiClient {
         &self,
         files: &[FileObj],
         format_advice: &[FormatAdvice],
-        tidy_advice: &[Vec<TidyNotification>],
+        tidy_advice: &[TidyAdvice],
         style: &str,
     ) {
         if !format_advice.is_empty() {
@@ -321,7 +320,7 @@ impl GithubApiClient {
         // The tidy_advice vector is parallel to the files vector; meaning it serves as a file filterer.
         // lines are already filter as specified to clang-tidy CLI.
         for (index, advice) in tidy_advice.iter().enumerate() {
-            for note in advice {
+            for note in &advice.notes {
                 if note.filename == files[index].name.to_string_lossy().replace('\\', "/") {
                     println!(
                         "::{severity} file={file},line={line},title={file}:{line}:{cols} [{diag}]::{info}",
@@ -344,13 +343,12 @@ impl GithubApiClient {
         url: &String,
         comment: &String,
         count: u64,
-        user_id: u64,
         no_lgtm: bool,
         is_lgtm: bool,
         update_only: bool,
     ) {
         let comment_url =
-            self.remove_bot_comments(url, user_id, count, !update_only || (is_lgtm && no_lgtm));
+            self.remove_bot_comments(url, count, !update_only || (is_lgtm && no_lgtm));
         #[allow(clippy::nonminimal_bool)] // an inaccurate assessment
         if (is_lgtm && !no_lgtm) || !is_lgtm {
             let payload = HashMap::from([("body", comment)]);
@@ -383,13 +381,7 @@ impl GithubApiClient {
         }
     }
 
-    fn remove_bot_comments(
-        &self,
-        url: &String,
-        count: u64,
-        user_id: u64,
-        delete: bool,
-    ) -> Option<String> {
+    fn remove_bot_comments(&self, url: &String, count: u64, delete: bool) -> Option<String> {
         let mut page = 1;
         let mut comment_url = None;
         let mut total = count;
@@ -402,9 +394,7 @@ impl GithubApiClient {
                 let payload: JsonCommentsPayload = response.json().unwrap();
                 let mut comment_count = 0;
                 for comment in payload.comments {
-                    if comment.body.starts_with("<!-- cpp linter action -->")
-                        && comment.user.id == user_id
-                    {
+                    if comment.body.starts_with(COMMENT_MARKER) {
                         log::debug!(
                             "comment id {} from user {} ({})",
                             comment.id,
diff --git a/cpp-linter-lib/src/rest_api/mod.rs b/cpp-linter-lib/src/rest_api/mod.rs
@@ -10,9 +10,33 @@ use reqwest::header::{HeaderMap, HeaderValue};
 
 // project specific modules/crates
 pub mod github_api;
-use crate::clang_tools::{clang_format::FormatAdvice, clang_tidy::TidyNotification};
+use crate::clang_tools::{clang_format::FormatAdvice, clang_tidy::TidyAdvice};
 use crate::common_fs::FileObj;
 
+pub static COMMENT_MARKER: &str = "<!-- cpp linter action -->";
+
+/// A struct to hold a collection of user inputs related to [`ResApiClient.post_feedback()`].
+pub struct FeedbackInput {
+    pub thread_comments: String,
+    pub no_lgtm: bool,
+    pub step_summary: bool,
+    pub file_annotations: bool,
+    pub style: String,
+}
+
+impl Default for FeedbackInput {
+    /// Construct a [`UserInput`] instance with default values.
+    fn default() -> Self {
+        FeedbackInput {
+            thread_comments: "false".to_string(),
+            no_lgtm: true,
+            step_summary: false,
+            file_annotations: true,
+            style: "llvm".to_string(),
+        }
+    }
+}
+
 /// A custom trait that templates necessary functionality with a Git server's REST API.
 pub trait RestApiClient {
     /// A way to set output variables specific to cpp_linter executions in CI.
@@ -53,7 +77,7 @@ pub trait RestApiClient {
         &self,
         files: &[FileObj],
         format_advice: &[FormatAdvice],
-        tidy_advice: &[Vec<TidyNotification>],
+        tidy_advice: &[TidyAdvice],
     ) -> (String, i32, i32) {
         let mut comment = String::from("<!-- cpp linter action -->\n# Cpp-Linter Report ");
         let mut format_checks_failed = 0;
@@ -73,8 +97,8 @@ pub trait RestApiClient {
         }
 
         let mut tidy_comment = String::new();
-        for (index, tidy_notes) in tidy_advice.iter().enumerate() {
-            for tidy_note in tidy_notes {
+        for (index, advice) in tidy_advice.iter().enumerate() {
+            for tidy_note in &advice.notes {
                 let file_path = PathBuf::from(&tidy_note.filename);
                 if file_path == files[index].name {
                     tidy_comment.push_str(&format!("- {}\n\n", tidy_note.filename));
@@ -84,7 +108,7 @@ pub trait RestApiClient {
                         line = tidy_note.line,
                         cols = tidy_note.cols,
                         severity = tidy_note.severity,
-                        diagnostic = tidy_note.diagnostic,
+                        diagnostic = tidy_note.diagnostic_link(),
                         rationale = tidy_note.rationale,
                         concerned_code = if tidy_note.suggestion.is_empty() {String::from("")} else {
                             format!("\n   ```{ext}\n   {suggestion}\n   ```\n",
@@ -122,16 +146,11 @@ pub trait RestApiClient {
     /// clang-format and clang-tidy (see `capture_clang_tools_output()`).
     ///
     /// All other parameters correspond to CLI arguments.
-    #[allow(clippy::too_many_arguments)]
     fn post_feedback(
         &self,
         files: &[FileObj],
         format_advice: &[FormatAdvice],
-        tidy_advice: &[Vec<TidyNotification>],
-        thread_comments: &str,
-        no_lgtm: bool,
-        step_summary: bool,
-        file_annotations: bool,
-        style: &str,
+        tidy_advice: &[TidyAdvice],
+        user_inputs: FeedbackInput,
     );
 }
diff --git a/cpp-linter-lib/src/run.rs b/cpp-linter-lib/src/run.rs
@@ -17,7 +17,7 @@ use crate::cli::{convert_extra_arg_val, get_arg_parser, parse_ignore};
 use crate::common_fs::{list_source_files, FileObj};
 use crate::github_api::GithubApiClient;
 use crate::logger::{self, end_log_group, start_log_group};
-use crate::rest_api::RestApiClient;
+use crate::rest_api::{FeedbackInput, RestApiClient};
 
 #[cfg(feature = "openssl-vendored")]
 fn probe_ssl_certs() {
@@ -111,32 +111,29 @@ pub fn run_main(args: Vec<String>) -> i32 {
     }
     end_log_group();
 
-    let style = args.get_one::<String>("style").unwrap();
+    let user_inputs = FeedbackInput {
+        style: args.get_one::<String>("style").unwrap().to_string(),
+        no_lgtm: args.get_flag("no-lgtm"),
+        step_summary: args.get_flag("step-summary"),
+        thread_comments: args
+            .get_one::<String>("thread-comments")
+            .unwrap()
+            .to_string(),
+        file_annotations: args.get_flag("file-annotations"),
+    };
+
     let extra_args = convert_extra_arg_val(&args);
     let (format_advice, tidy_advice) = capture_clang_tools_output(
         &files,
         args.get_one::<String>("version").unwrap(),
         args.get_one::<String>("tidy-checks").unwrap(),
-        style,
+        user_inputs.style.as_str(),
         lines_changed_only,
         database_path,
         extra_args,
     );
     start_log_group(String::from("Posting feedback"));
-    let no_lgtm = args.get_flag("no-lgtm");
-    let step_summary = args.get_flag("step-summary");
-    let thread_comments = args.get_one::<String>("thread-comments").unwrap();
-    let file_annotations = args.get_flag("file-annotations");
-    rest_api_client.post_feedback(
-        &files,
-        &format_advice,
-        &tidy_advice,
-        thread_comments,
-        no_lgtm,
-        step_summary,
-        file_annotations,
-        style,
-    );
+    rest_api_client.post_feedback(&files, &format_advice, &tidy_advice, user_inputs);
     end_log_group();
     0
 }
