diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index e4d61d2..3de3c3c 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -1,6 +1,6 @@ repos: - repo: https://github.com/pre-commit/pre-commit-hooks - rev: v4.5.0 + rev: v4.6.0 hooks: - id: trailing-whitespace exclude: cpp-linter-lib/tests/capture_tools_output/cpp-linter/cpp-linter/test_git_lib.patch @@ -14,7 +14,7 @@ repos: - id: mixed-line-ending args: ["--fix=lf"] - repo: https://github.com/astral-sh/ruff-pre-commit - rev: v0.1.8 + rev: v0.5.2 hooks: # Run the python linter. - id: ruff diff --git a/cpp-linter-lib/src/clang_tools/clang_format.rs b/cpp-linter-lib/src/clang_tools/clang_format.rs index 72b860e..22154a0 100644 --- a/cpp-linter-lib/src/clang_tools/clang_format.rs +++ b/cpp-linter-lib/src/clang_tools/clang_format.rs @@ -8,7 +8,10 @@ use serde::Deserialize; use serde_xml_rs::de::Deserializer; // project-specific crates/modules -use crate::common_fs::{get_line_cols_from_offset, FileObj}; +use crate::{ + cli::LinesChangedOnly, + common_fs::{get_line_cols_from_offset, FileObj}, +}; /// A Structure used to deserialize clang-format's XML output. #[derive(Debug, Deserialize, PartialEq)] @@ -62,7 +65,7 @@ pub fn run_clang_format( cmd: &mut Command, file: &FileObj, style: &str, - lines_changed_only: u8, + lines_changed_only: &LinesChangedOnly, ) -> FormatAdvice { cmd.args(["--style", style, "--output-replacements-xml"]); let ranges = file.get_ranges(lines_changed_only); diff --git a/cpp-linter-lib/src/clang_tools/clang_tidy.rs b/cpp-linter-lib/src/clang_tools/clang_tidy.rs index ea260a1..a99639e 100644 --- a/cpp-linter-lib/src/clang_tools/clang_tidy.rs +++ b/cpp-linter-lib/src/clang_tools/clang_tidy.rs @@ -12,7 +12,10 @@ use regex::Regex; use serde::Deserialize; // project-specific modules/crates -use crate::common_fs::{normalize_path, FileObj}; +use crate::{ + cli::LinesChangedOnly, + common_fs::{normalize_path, FileObj}, +}; /// Used to deserialize a JSON compilation database #[derive(Deserialize, Debug)] @@ -68,6 +71,26 @@ pub struct TidyNotification { pub suggestion: Vec, } +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, +} + /// Parses clang-tidy stdout. /// /// Here it helps to have the JSON database deserialized for normalizing paths present @@ -75,7 +98,7 @@ pub struct TidyNotification { fn parse_tidy_output( tidy_stdout: &[u8], database_json: &Option, -) -> Vec { +) -> 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(); @@ -141,7 +164,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. @@ -149,11 +172,11 @@ pub fn run_clang_tidy( cmd: &mut Command, file: &FileObj, checks: &str, - lines_changed_only: u8, + lines_changed_only: &LinesChangedOnly, database: &Option, extra_args: &Option>, database_json: &Option, -) -> Vec { +) -> TidyAdvice { if !checks.is_empty() { cmd.args(["-checks", checks]); } @@ -165,7 +188,7 @@ pub fn run_clang_tidy( cmd.args(["--extra-arg", format!("\"{}\"", arg).as_str()]); } } - if lines_changed_only > 0 { + if *lines_changed_only != LinesChangedOnly::Off { let ranges = file.get_ranges(lines_changed_only); let filter = format!( "[{{\"name\":{:?},\"lines\":{:?}}}]", diff --git a/cpp-linter-lib/src/clang_tools/mod.rs b/cpp-linter-lib/src/clang_tools/mod.rs index c44adba..637aa4a 100644 --- a/cpp-linter-lib/src/clang_tools/mod.rs +++ b/cpp-linter-lib/src/clang_tools/mod.rs @@ -10,11 +10,14 @@ use which::{which, which_in}; // project-specific modules/crates use super::common_fs::FileObj; -use crate::logger::{end_log_group, start_log_group}; +use crate::{ + cli::LinesChangedOnly, + 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 +72,7 @@ pub fn get_clang_tool_exe(name: &str, version: &str) -> Result, extra_args: Option>, -) -> (Vec, Vec>) { +) -> (Vec, Vec) { // 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 +129,8 @@ pub fn capture_clang_tools_output( }; // iterate over the discovered files and run the clang tools - let mut all_format_advice: Vec = Vec::with_capacity(files.len()); - let mut all_tidy_advice: Vec> = - Vec::with_capacity(files.len()); + let mut all_format_advice: Vec = Vec::with_capacity(files.len()); + let mut all_tidy_advice: Vec = 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/cli.rs b/cpp-linter-lib/src/cli.rs index 23d624b..ef8a17c 100644 --- a/cpp-linter-lib/src/cli.rs +++ b/cpp-linter-lib/src/cli.rs @@ -6,6 +6,17 @@ use std::fs; use clap::builder::FalseyValueParser; use clap::{Arg, ArgAction, ArgMatches, Command}; +/// An enum to describe `--lines-changed-only` CLI option's behavior. +#[derive(PartialEq)] +pub enum LinesChangedOnly { + /// All lines are scanned + Off, + /// Only lines in the diff are scanned + Diff, + /// Only lines in the diff with additions are scanned. + On, +} + /// Builds and returns the Command Line Interface's argument parsing object. pub fn get_arg_parser() -> Command { Command::new("cpp-linter") diff --git a/cpp-linter-lib/src/common_fs.rs b/cpp-linter-lib/src/common_fs.rs index afdd4a0..fcc0151 100644 --- a/cpp-linter-lib/src/common_fs.rs +++ b/cpp-linter-lib/src/common_fs.rs @@ -5,6 +5,8 @@ use std::path::{Component, Path}; use std::{fs, io}; use std::{ops::RangeInclusive, path::PathBuf}; +use crate::cli::LinesChangedOnly; + /// A structure to represent a file's path and line changes. #[derive(Debug)] pub struct FileObj { @@ -52,7 +54,7 @@ impl FileObj { /// A helper function to consolidate a [Vec] of line numbers into a /// [Vec>] in which each range describes the beginning and /// ending of a group of consecutive line numbers. - fn consolidate_numbers_to_ranges(lines: &Vec) -> Vec> { + fn consolidate_numbers_to_ranges(lines: &[u32]) -> Vec> { let mut range_start = None; let mut ranges: Vec> = Vec::new(); for (index, number) in lines.iter().enumerate() { @@ -69,13 +71,11 @@ impl FileObj { ranges } - pub fn get_ranges(&self, lines_changed_only: u8) -> Vec> { - if lines_changed_only == 2 { - self.diff_chunks.to_vec() - } else if lines_changed_only == 1 { - self.added_ranges.to_vec() - } else { - Vec::new() + pub fn get_ranges(&self, lines_changed_only: &LinesChangedOnly) -> Vec> { + match lines_changed_only { + LinesChangedOnly::Diff => self.diff_chunks.to_vec(), + LinesChangedOnly::On => self.added_ranges.to_vec(), + _ => Vec::new(), } } } @@ -249,6 +249,7 @@ mod test { use std::path::PathBuf; use super::{get_line_cols_from_offset, list_source_files, normalize_path, FileObj}; + use crate::cli::LinesChangedOnly; use crate::cli::{get_arg_parser, parse_ignore}; use crate::common_fs::is_file_in_list; @@ -406,7 +407,7 @@ mod test { #[test] fn get_ranges_0() { let file_obj = FileObj::new(PathBuf::from("tests/demo/demo.cpp")); - let ranges = file_obj.get_ranges(0); + let ranges = file_obj.get_ranges(&LinesChangedOnly::Off); assert!(ranges.is_empty()); } @@ -419,7 +420,7 @@ mod test { added_lines, diff_chunks.clone(), ); - let ranges = file_obj.get_ranges(2); + let ranges = file_obj.get_ranges(&LinesChangedOnly::Diff); assert_eq!(ranges, diff_chunks); } @@ -432,7 +433,7 @@ mod test { added_lines, diff_chunks, ); - let ranges = file_obj.get_ranges(1); + let ranges = file_obj.get_ranges(&LinesChangedOnly::On); assert_eq!(ranges, vec![4..=5, 9..=9]); } } diff --git a/cpp-linter-lib/src/rest_api/github_api.rs b/cpp-linter-lib/src/rest_api/github_api.rs index 19418b9..cbe7b0e 100644 --- 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}; /// A structure to work with Github REST API. pub struct GithubApiClient { @@ -130,7 +130,7 @@ impl RestApiClient for GithubApiClient { + if use_diff.is_some_and(|val| val) { "diff" } else { - "text+json" + "raw+json" }; headers.insert("Accept", return_fmt.parse().unwrap()); let user_agent = @@ -187,16 +187,30 @@ impl RestApiClient for GithubApiClient { &self, files: &[FileObj], format_advice: &[FormatAdvice], - tidy_advice: &[Vec], - 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.file_annotations { + self.post_annotations( + files, + format_advice, + tidy_advice, + user_inputs.style.as_str(), + ); + } + if user_inputs.step_summary { + self.post_step_summary(&comment); + } + self.set_exit_code( + format_checks_failed + tidy_checks_failed, + Some(format_checks_failed), + Some(tidy_checks_failed), + ); + + 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"; @@ -223,15 +237,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(); @@ -246,17 +258,6 @@ impl RestApiClient for GithubApiClient { } } } - if file_annotations { - self.post_annotations(files, format_advice, tidy_advice, style); - } - if step_summary { - self.post_step_summary(&comment); - } - self.set_exit_code( - format_checks_failed + tidy_checks_failed, - Some(format_checks_failed), - Some(tidy_checks_failed), - ); } } @@ -277,7 +278,7 @@ impl GithubApiClient { &self, files: &[FileObj], format_advice: &[FormatAdvice], - tidy_advice: &[Vec], + tidy_advice: &[TidyAdvice], style: &str, ) { if !format_advice.is_empty() { @@ -322,7 +323,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}", @@ -345,13 +346,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)]); @@ -384,13 +384,7 @@ impl GithubApiClient { } } - fn remove_bot_comments( - &self, - url: &String, - count: u64, - user_id: u64, - delete: bool, - ) -> Option { + fn remove_bot_comments(&self, url: &String, count: u64, delete: bool) -> Option { let mut page = 1; let mut comment_url = None; let mut total = count; @@ -403,9 +397,7 @@ impl GithubApiClient { let payload: JsonCommentsPayload = response.json().unwrap(); let mut comment_count = 0; for comment in payload.comments { - if comment.body.starts_with("") - && 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 index 4887176..4a4fa86 100644 --- 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 = ""; + +/// 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], + tidy_advice: &[TidyAdvice], ) -> (String, i32, i32) { let mut comment = String::from("\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], - 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 index f1120bb..3c428ed 100644 --- a/cpp-linter-lib/src/run.rs +++ b/cpp-linter-lib/src/run.rs @@ -13,11 +13,11 @@ use openssl_probe; // project specific modules/crates use crate::clang_tools::capture_clang_tools_output; -use crate::cli::{convert_extra_arg_val, get_arg_parser, parse_ignore}; +use crate::cli::{convert_extra_arg_val, get_arg_parser, parse_ignore, LinesChangedOnly}; 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() { @@ -90,15 +90,14 @@ pub fn run_main(args: Vec) -> i32 { .unwrap() .as_str() { - "false" => 0, - "true" => 1, - "diff" => 2, - _ => unreachable!(), + "true" => LinesChangedOnly::On, + "diff" => LinesChangedOnly::Diff, + _ => LinesChangedOnly::Off, }; let files_changed_only = args.get_flag("files-changed-only"); start_log_group(String::from("Get list of specified source files")); - let files: Vec = if lines_changed_only != 0 || files_changed_only { + let files: Vec = if lines_changed_only != LinesChangedOnly::Off || files_changed_only { // parse_diff(github_rest_api_payload) rest_api_client.get_list_of_changed_files(&extensions, &ignored, ¬_ignored) } else { @@ -111,32 +110,29 @@ pub fn run_main(args: Vec) -> i32 { } end_log_group(); - let style = args.get_one::("style").unwrap(); + let user_inputs = FeedbackInput { + style: args.get_one::("style").unwrap().to_string(), + no_lgtm: args.get_flag("no-lgtm"), + step_summary: args.get_flag("step-summary"), + thread_comments: args + .get_one::("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::("version").unwrap(), args.get_one::("tidy-checks").unwrap(), - style, - lines_changed_only, + 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::("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 } diff --git a/cpp-linter-py/cpp_linter/entry_point.py b/cpp-linter-py/cpp_linter/entry_point.py index faf01fe..d550630 100644 --- a/cpp-linter-py/cpp_linter/entry_point.py +++ b/cpp-linter-py/cpp_linter/entry_point.py @@ -15,6 +15,7 @@ point alias ("path/to/cpp-linter.exe"). Thus, the parser in `cli.rs` will halt on an error because it is not configured to handle positional arguments. """ + import sys # Using relative import to load binary lib with same name as root package.