From 0fbd22d2d1bcb323703020829ff7ba4d0554698b Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Tue, 20 Aug 2024 17:37:01 -0700 Subject: [PATCH 1/3] implement positional arg files begin pre-commit support --- .gitattributes | 1 + cpp-linter-lib/src/cli.rs | 83 ++++++++++++--------- cpp-linter-lib/src/common_fs/file_filter.rs | 6 +- cpp-linter-lib/src/rest_api/github_api.rs | 7 +- cpp-linter-lib/src/rest_api/mod.rs | 2 +- cpp-linter-lib/src/run.rs | 12 ++- docs/src/main.rs | 13 +++- 7 files changed, 79 insertions(+), 45 deletions(-) diff --git a/.gitattributes b/.gitattributes index ef12704..d559398 100644 --- a/.gitattributes +++ b/.gitattributes @@ -25,3 +25,4 @@ *.code-workspace text eol=lf *.clang-tidy text eol=lf *.clang-format text eol=lf +justfile text eol=lf diff --git a/cpp-linter-lib/src/cli.rs b/cpp-linter-lib/src/cli.rs index 745fbd5..60bcd47 100644 --- a/cpp-linter-lib/src/cli.rs +++ b/cpp-linter-lib/src/cli.rs @@ -28,10 +28,10 @@ pub fn get_arg_parser() -> Command { .short('v') .default_value("info") .value_parser(["debug", "info"]) - .long_help( + .help( "This controls the action's verbosity in the workflow's logs. This option does not affect the verbosity of resulting -thread comments or file annotations.", +thread comments or file annotations.\n\n", ), ) .arg( @@ -39,7 +39,7 @@ thread comments or file annotations.", .long("database") .short('p') .help_heading("clang-tidy options") - .long_help( + .help( "The path that is used to read a compile command database. For example, it can be a CMake build directory in which a file named compile_commands.json exists (set `CMAKE_EXPORT_COMPILE_COMMANDS` to `ON`). @@ -54,13 +54,13 @@ for an example of setting up Clang Tooling on a source tree.", .long("style") .default_value("llvm") .help_heading("clang-format options") - .long_help( + .help( "The style rules to use. - Set this to `file` to have clang-format use the closest relative .clang-format file. - Set this to a blank string (`''`) to disable using clang-format - entirely.", + entirely.\n\n", ), ) .arg( @@ -71,7 +71,7 @@ for an example of setting up Clang Tooling on a source tree.", "boost-*,bugprone-*,performance-*,readability-*,portability-*,modernize-*,clang-analyzer-*,cppcoreguidelines-*", ) .help_heading("clang-tidy options") - .long_help( + .help( "A comma-separated list of globs with optional `-` prefix. Globs are processed in order of appearance in the list. Globs without `-` prefix add checks with matching names to the set, @@ -84,7 +84,7 @@ option in a .clang-tidy file (if any). - It is also possible to rely solely on a .clang-tidy config file by specifying this option as a blank string (`''`). -See also clang-tidy docs for more info.", +See also clang-tidy docs for more info.\n\n", ), ) .arg( @@ -95,7 +95,7 @@ See also clang-tidy docs for more info.", .num_args(0..=1) .require_equals(true) .default_value("") - .long_help( + .help( "The desired version of the clang tools to use. Accepted options are strings which can be 8, 9, 10, 11, 12, 13, 14, 15, 16, 17. @@ -103,7 +103,7 @@ strings which can be 8, 9, 10, 11, 12, 13, 14, 15, 16, 17. platform's default installed version. - This value can also be a path to where the clang tools are installed (if using a custom install location). All paths specified - here are converted to absolute.", + here are converted to absolute.\n\n", ), ) .arg( @@ -112,19 +112,19 @@ strings which can be 8, 9, 10, 11, 12, 13, 14, 15, 16, 17. .long("extensions") .value_delimiter(',') .default_value("c,h,C,H,cpp,hpp,cc,hh,c++,h++,cxx,hxx") - .help_heading("source options") - .long_help("A comma-separated list of file extensions to analyze."), + .help_heading("Source options") + .help("A comma-separated list of file extensions to analyze.\n"), ) .arg( Arg::new("repo-root") .short('r') .long("repo-root") .default_value(".") - .help_heading("source options") - .long_help( + .help_heading("Source options") + .help( "The relative path to the repository root directory. This path is relative to the runner's `GITHUB_WORKSPACE` environment variable (or -the current working directory if not using a CI runner).", +the current working directory if not using a CI runner).\n\n", ), ) .arg( @@ -133,8 +133,8 @@ the current working directory if not using a CI runner).", .long("ignore") .value_delimiter('|') .default_value(".github|target") - .help_heading("source options") - .long_help( + .help_heading("Source options") + .help( "Set this option with path(s) to ignore (or not ignore). - In the case of multiple paths, you can use `|` to separate each path. @@ -147,7 +147,7 @@ the current working directory if not using a CI runner).", - Prefix a path with `!` to explicitly not ignore it. This can be applied to a submodule's path (if desired) but not hidden directories. - Glob patterns are not supported here. All asterisk characters (`*`) - are literal.", + are literal.\n\n", ), ) .arg( @@ -157,9 +157,9 @@ the current working directory if not using a CI runner).", .value_delimiter('|') .default_value("") .help_heading("clang-tidy options") - .long_help( + .help( "Similar to [`--ignore`](#-i---ignore) but applied -exclusively to files analyzed by clang-tidy.", +exclusively to files analyzed by clang-tidy.\n\n", ), ) .arg( @@ -169,9 +169,9 @@ exclusively to files analyzed by clang-tidy.", .value_delimiter('|') .default_value("") .help_heading("clang-format options") - .long_help( + .help( "Similar to [`--ignore`](#-i---ignore) but applied -exclusively to files analyzed by clang-format.", +exclusively to files analyzed by clang-format.\n\n", ), ) .arg( @@ -180,15 +180,15 @@ exclusively to files analyzed by clang-format.", .long("lines-changed-only") .value_parser(["true", "false", "diff"]) .default_value("true") - .help_heading("source options") - .long_help( + .help_heading("Source options") + .help( "This controls what part of the files are analyzed. The following values are accepted: - `false`: All lines in a file are analyzed. - `true`: Only lines in the diff that contain additions are analyzed. - `diff`: All lines in the diff are analyzed (including unchanged - lines but not subtractions).", + lines but not subtractions).\n\n", ), ) .arg( @@ -198,8 +198,8 @@ The following values are accepted: .default_value_if("lines-changed-only", ArgPredicate::Equals("true".into()), "true") .default_value("false") .value_parser(FalseyValueParser::new()) - .help_heading("source options") - .long_help( + .help_heading("Source options") + .help( "Set this option to false to analyze any source files in the repo. This is automatically enabled if [`--lines-changed-only`](#-l---lines-changed-only) is enabled. @@ -210,7 +210,7 @@ This is automatically enabled if > does not not have the privilege to list the changed files for an event. > > See [Authenticating with the `GITHUB_TOKEN`]( -> https://docs.github.com/en/actions/reference/authentication-in-a-workflow).", +> https://docs.github.com/en/actions/reference/authentication-in-a-workflow).\n\n", ), ) .arg( @@ -219,7 +219,7 @@ This is automatically enabled if .short('x') .action(ArgAction::Append) .help_heading("clang-tidy options") - .long_help( + .help( "A string of extra arguments passed to clang-tidy for use as compiler arguments. This can be specified more than once for each additional argument. Recommend using quotes around the value and @@ -237,7 +237,7 @@ cpp-linter --extra-arg=\"-std=c++17\" --extra-arg=\"-Wall\" .value_parser(["true", "false", "updated"]) .default_value("false") .help_heading("feedback options") - .long_help( + .help( "Set this option to true to enable the use of thread comments as feedback. Set this to `update` to update an existing comment if one exists; the value 'true' will always delete an old comment and post a new one if necessary. @@ -248,7 +248,7 @@ the value 'true' will always delete an old comment and post a new one if necessa > variable. > > See [Authenticating with the `GITHUB_TOKEN`]( -> https://docs.github.com/en/actions/reference/authentication-in-a-workflow).", +> https://docs.github.com/en/actions/reference/authentication-in-a-workflow).\n\n", ), ) .arg( @@ -258,13 +258,13 @@ the value 'true' will always delete an old comment and post a new one if necessa .value_parser(FalseyValueParser::new()) .default_value("true") .help_heading("feedback options") - .long_help( + .help( "Set this option to true or false to enable or disable the use of a thread comment that basically says 'Looks Good To Me' (when all checks pass). > [!important] > The [`--thread-comments`](#-g---thread-comments) -> option also notes further implications.", +> option also notes further implications.\n\n", ), ) .arg( @@ -274,9 +274,9 @@ thread comment that basically says 'Looks Good To Me' (when all checks pass). .value_parser(FalseyValueParser::new()) .default_value("false") .help_heading("feedback options") - .long_help( + .help( "Set this option to true or false to enable or disable the use of -a workflow step summary when the run has concluded.", +a workflow step summary when the run has concluded.\n\n", ), ) .arg( @@ -286,11 +286,21 @@ a workflow step summary when the run has concluded.", .value_parser(FalseyValueParser::new()) .default_value("true") .help_heading("feedback options") - .long_help( + .help( "Set this option to false to disable the use of -file annotations as feedback.", +file annotations as feedback.\n\n", ), ) + .arg( + Arg::new("files") + .action(ArgAction::Append) + .help( + "An explicit path to a file. +This can be specified zero or more times, resulting in a list of files. +The list of files is appended to the internal list of 'not ignored' files. +Further filtering can still be applied (see [Source options](#source-options)).", + ) + ) .groups([ ArgGroup::new("Clang-tidy options") .args(["tidy-checks", "database", "extra-arg", "ignore-tidy"]), @@ -301,6 +311,7 @@ file annotations as feedback.", "thread-comments", "no-lgtm", "step-summary", "file-annotations" ]), ]) + .next_line_help(true) } /// Converts the parsed value of the `--extra-arg` option into an optional vector of strings. diff --git a/cpp-linter-lib/src/common_fs/file_filter.rs b/cpp-linter-lib/src/common_fs/file_filter.rs index 8605e27..975f9fa 100644 --- a/cpp-linter-lib/src/common_fs/file_filter.rs +++ b/cpp-linter-lib/src/common_fs/file_filter.rs @@ -4,9 +4,9 @@ use super::FileObj; #[derive(Debug, Clone)] pub struct FileFilter { - ignored: Vec, - not_ignored: Vec, - extensions: Vec, + pub ignored: Vec, + pub not_ignored: Vec, + pub extensions: Vec, } impl FileFilter { pub fn new(ignore: &[&str], extensions: Vec) -> Self { diff --git a/cpp-linter-lib/src/rest_api/github_api.rs b/cpp-linter-lib/src/rest_api/github_api.rs index c2bf05d..12ec1a7 100644 --- a/cpp-linter-lib/src/rest_api/github_api.rs +++ b/cpp-linter-lib/src/rest_api/github_api.rs @@ -191,7 +191,11 @@ impl RestApiClient for GithubApiClient { } } - async fn post_feedback(&self, files: &[Arc>], user_inputs: FeedbackInput) { + async fn post_feedback( + &self, + files: &[Arc>], + user_inputs: FeedbackInput, + ) -> u64 { let format_checks_failed = tally_format_advice(files); let tidy_checks_failed = tally_tidy_advice(files); let mut comment = None; @@ -268,6 +272,7 @@ impl RestApiClient for GithubApiClient { } } } + format_checks_failed + tidy_checks_failed } } diff --git a/cpp-linter-lib/src/rest_api/mod.rs b/cpp-linter-lib/src/rest_api/mod.rs index 675a20a..dc2437a 100644 --- a/cpp-linter-lib/src/rest_api/mod.rs +++ b/cpp-linter-lib/src/rest_api/mod.rs @@ -124,7 +124,7 @@ pub trait RestApiClient { &self, files: &[Arc>], user_inputs: FeedbackInput, - ) -> impl Future; + ) -> impl Future; } fn make_format_comment( diff --git a/cpp-linter-lib/src/run.rs b/cpp-linter-lib/src/run.rs index 49dda3a..1bca001 100644 --- a/cpp-linter-lib/src/run.rs +++ b/cpp-linter-lib/src/run.rs @@ -100,6 +100,13 @@ pub async fn run_main(args: Vec) -> i32 { .collect::>(); let mut file_filter = FileFilter::new(&ignore, extensions.clone()); file_filter.parse_submodules(); + if let Some(files) = args.get_many::>("files") { + for list in files { + file_filter + .not_ignored + .extend(list.iter().map(|v| v.to_string()).collect::>()); + } + } let lines_changed_only = match args .get_one::("lines-changed-only") @@ -166,8 +173,11 @@ pub async fn run_main(args: Vec) -> i32 { }; capture_clang_tools_output(&mut arc_files, version, &mut clang_params).await; start_log_group(String::from("Posting feedback")); - rest_api_client.post_feedback(&arc_files, user_inputs).await; + let checks_failed = rest_api_client.post_feedback(&arc_files, user_inputs).await; end_log_group(); + if env::var("PRE_COMMIT").is_ok_and(|v| v == "1") { + return (checks_failed > 1) as i32; + } 0 } diff --git a/docs/src/main.rs b/docs/src/main.rs index e2199dc..6a1ce24 100644 --- a/docs/src/main.rs +++ b/docs/src/main.rs @@ -99,6 +99,13 @@ mod cli_gen_lib { format!("{}\n", &cmd.get_about().unwrap().to_string().trim()).as_str(), ); } + out.push_str("## Arguments\n"); + for arg in command.get_positionals() { + out.push_str(format!("\n### `{}`\n\n", arg.get_id().as_str()).as_str()); + if let Some(help) = arg.get_help() { + out.push_str(format!("{}\n", help.to_string().trim()).as_str()); + } + } let arg_groups = if let Some(groups) = groups_order { eprintln!("ordering groups into {:?}", groups); let mut ordered = Vec::with_capacity(command.get_groups().count()); @@ -146,9 +153,9 @@ mod cli_gen_lib { .as_str(), ); } - out.push_str( - format!("{}\n", &arg.get_long_help().unwrap().to_string().trim()).as_str(), - ); + if let Some(help) = &arg.get_help() { + out.push_str(format!("{}\n", help.to_string().trim()).as_str()); + } } } out From 1b66ad6265fe0f9d105b7189673c6334f844a97d Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Wed, 21 Aug 2024 08:26:59 -0700 Subject: [PATCH 2/3] deserialize CLI options into structs from parsed results --- cpp-linter-lib/Cargo.toml | 2 +- cpp-linter-lib/README.md | 14 +- cpp-linter-lib/examples/gh_rest_api.rs | 2 +- cpp-linter-lib/src/clang_tools/mod.rs | 17 +- cpp-linter-lib/src/{cli.rs => cli/mod.rs} | 45 ++--- cpp-linter-lib/src/cli/structs.rs | 198 ++++++++++++++++++++ cpp-linter-lib/src/common_fs/file_filter.rs | 19 +- cpp-linter-lib/src/git.rs | 11 +- cpp-linter-lib/src/rest_api/github_api.rs | 15 +- cpp-linter-lib/src/rest_api/mod.rs | 23 +-- cpp-linter-lib/src/run.rs | 107 +++-------- cspell.config.yml | 1 + justfile | 2 +- 13 files changed, 277 insertions(+), 179 deletions(-) rename cpp-linter-lib/src/{cli.rs => cli/mod.rs} (92%) create mode 100644 cpp-linter-lib/src/cli/structs.rs diff --git a/cpp-linter-lib/Cargo.toml b/cpp-linter-lib/Cargo.toml index f2ef9c6..07c3ebb 100644 --- a/cpp-linter-lib/Cargo.toml +++ b/cpp-linter-lib/Cargo.toml @@ -8,7 +8,7 @@ documentation.workspace = true # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html [dependencies] -clap = { version = "4.5.16" } +clap = "4.5.16" git2 = "0.19.0" lenient_semver = "0.4.2" log = "0.4.22" diff --git a/cpp-linter-lib/README.md b/cpp-linter-lib/README.md index 0427147..7e8adbe 100644 --- a/cpp-linter-lib/README.md +++ b/cpp-linter-lib/README.md @@ -2,11 +2,19 @@ # cpp-linter-lib This crate contains the the library used as a backend for the -`cpp-linter` binary executable. +`cpp-linter` binary executable. The main focus of `cpp-linter` is as follows: + +- [x] Lint C/C++ sources using clang-format and clang-tidy. +- [x] Respect file changes when run in a CI workflow on Github. +- [x] Provide feedback via Github's REST API in the any of the following forms: + - [x] step summary + - [x] thread comments + - [x] file annotation + - [ ] pull request review suggestions Since the [cpp-linter python package][pypi-org] now uses this library -as a binding, the native binary's `main()` method is also present in this -library. +as a binding, the native binary's `main()` behavior is also present in this +library (see `run::run_main()`). See also the [CLI document hosted on github][gh-pages]. diff --git a/cpp-linter-lib/examples/gh_rest_api.rs b/cpp-linter-lib/examples/gh_rest_api.rs index dc88022..1ec8b94 100644 --- a/cpp-linter-lib/examples/gh_rest_api.rs +++ b/cpp-linter-lib/examples/gh_rest_api.rs @@ -14,7 +14,7 @@ pub async fn main() -> Result<(), Box> { let client_controller = GithubApiClient::new(); let file_filter = FileFilter::new( - &["target", ".github"], + &["target".to_string(), ".github".to_string()], vec!["cpp".to_string(), "hpp".to_string()], ); diff --git a/cpp-linter-lib/src/clang_tools/mod.rs b/cpp-linter-lib/src/clang_tools/mod.rs index 31e9145..5f47cf0 100644 --- a/cpp-linter-lib/src/clang_tools/mod.rs +++ b/cpp-linter-lib/src/clang_tools/mod.rs @@ -18,8 +18,7 @@ use which::{which, which_in}; // project-specific modules/crates use super::common_fs::FileObj; use crate::{ - cli::LinesChangedOnly, - common_fs::FileFilter, + cli::ClangParams, logger::{end_log_group, start_log_group}, }; pub mod clang_format; @@ -77,20 +76,6 @@ pub fn get_clang_tool_exe(name: &str, version: &str) -> Result, - pub extra_args: Option>, - pub database_json: Option, - pub style: String, - pub clang_tidy_command: Option, - pub clang_format_command: Option, - pub tidy_filter: FileFilter, - pub format_filter: FileFilter, -} - /// This creates a task to run clang-tidy and clang-format on a single file. /// /// Returns a Future that infallibly resolves to a 2-tuple that contains diff --git a/cpp-linter-lib/src/cli.rs b/cpp-linter-lib/src/cli/mod.rs similarity index 92% rename from cpp-linter-lib/src/cli.rs rename to cpp-linter-lib/src/cli/mod.rs index 60bcd47..e75579d 100644 --- a/cpp-linter-lib/src/cli.rs +++ b/cpp-linter-lib/src/cli/mod.rs @@ -1,19 +1,12 @@ //! This module holds the Command Line Interface design. +use std::path::PathBuf; // non-std crates use clap::builder::{ArgPredicate, FalseyValueParser}; -use clap::{Arg, ArgAction, ArgGroup, ArgMatches, Command}; +use clap::{value_parser, Arg, ArgAction, ArgGroup, ArgMatches, Command}; -/// An enum to describe `--lines-changed-only` CLI option's behavior. -#[derive(PartialEq, Clone, Debug)] -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, -} +mod structs; +pub use structs::{ClangParams, Cli, FeedbackInput, LinesChangedOnly, ThreadComments}; /// Builds and returns the Command Line Interface's argument parsing object. pub fn get_arg_parser() -> Command { @@ -38,9 +31,10 @@ thread comments or file annotations.\n\n", Arg::new("database") .long("database") .short('p') + .value_parser(value_parser!(PathBuf)) .help_heading("clang-tidy options") .help( - "The path that is used to read a compile command database. + "The path that is used to read a compile command database. For example, it can be a CMake build directory in which a file named compile_commands.json exists (set `CMAKE_EXPORT_COMPILE_COMMANDS` to `ON`). When no build path is specified, a search for compile_commands.json will be @@ -155,7 +149,6 @@ the current working directory if not using a CI runner).\n\n", .short('D') .long("ignore-tidy") .value_delimiter('|') - .default_value("") .help_heading("clang-tidy options") .help( "Similar to [`--ignore`](#-i---ignore) but applied @@ -167,7 +160,6 @@ exclusively to files analyzed by clang-tidy.\n\n", .short('M') .long("ignore-format") .value_delimiter('|') - .default_value("") .help_heading("clang-format options") .help( "Similar to [`--ignore`](#-i---ignore) but applied @@ -178,7 +170,7 @@ exclusively to files analyzed by clang-format.\n\n", Arg::new("lines-changed-only") .short('l') .long("lines-changed-only") - .value_parser(["true", "false", "diff"]) + .value_parser(["true", "on", "1", "false", "off", "0", "diff"]) .default_value("true") .help_heading("Source options") .help( @@ -220,21 +212,21 @@ This is automatically enabled if .action(ArgAction::Append) .help_heading("clang-tidy options") .help( - "A string of extra arguments passed to clang-tidy for use as + r#"A string of extra arguments passed to clang-tidy for use as compiler arguments. This can be specified more than once for each additional argument. Recommend using quotes around the value and avoid using spaces between name and value (use `=` instead): ```shell -cpp-linter --extra-arg=\"-std=c++17\" --extra-arg=\"-Wall\" -```", +cpp-linter --extra-arg="-std=c++17" --extra-arg="-Wall" +```"#, ), ) .arg( Arg::new("thread-comments") .long("thread-comments") .short('g') - .value_parser(["true", "false", "updated"]) + .value_parser(["true", "on", "1", "false", "off", "0", "updated"]) .default_value("false") .help_heading("feedback options") .help( @@ -381,11 +373,9 @@ mod test { fn extra_arg_1() { let args = parser_args(vec!["cpp-linter", "--extra-arg='-std=c++17 -Wall'"]); let extras = convert_extra_arg_val(&args); - assert!(extras.is_some()); - if let Some(extra_args) = extras { - assert_eq!(extra_args.len(), 2); - assert_eq!(extra_args, ["-std=c++17", "-Wall"]) - } + let extra_args = extras.expect("extra-arg not parsed properly"); + assert_eq!(extra_args.len(), 2); + assert_eq!(extra_args, ["-std=c++17", "-Wall"]) } #[test] @@ -397,9 +387,8 @@ mod test { ]); let extras = convert_extra_arg_val(&args); assert!(extras.is_some()); - if let Some(extra_args) = extras { - assert_eq!(extra_args.len(), 2); - assert_eq!(extra_args, ["-std=c++17", "-Wall"]) - } + let extra_args = extras.expect("extra-arg not parsed properly"); + assert_eq!(extra_args.len(), 2); + assert_eq!(extra_args, ["-std=c++17", "-Wall"]) } } diff --git a/cpp-linter-lib/src/cli/structs.rs b/cpp-linter-lib/src/cli/structs.rs new file mode 100644 index 0000000..769c36d --- /dev/null +++ b/cpp-linter-lib/src/cli/structs.rs @@ -0,0 +1,198 @@ +use std::path::PathBuf; + +use clap::ArgMatches; + +use super::convert_extra_arg_val; +use crate::{clang_tools::clang_tidy::CompilationDatabase, common_fs::FileFilter}; + +/// An enum to describe `--lines-changed-only` CLI option's behavior. +#[derive(PartialEq, Clone, Debug)] +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, +} + +impl LinesChangedOnly { + fn from_string(val: &str) -> LinesChangedOnly { + match val { + "true" | "on" | "1" => LinesChangedOnly::On, + "diff" => LinesChangedOnly::Diff, + _ => LinesChangedOnly::Off, + } + } +} + +/// A structure to contain parsed CLI options. +pub struct Cli { + pub version: String, + pub verbosity: bool, + pub extensions: Vec, + pub repo_root: String, + pub lines_changed_only: LinesChangedOnly, + pub files_changed_only: bool, + pub ignore: Vec, + pub style: String, + pub ignore_format: Vec, + pub ignore_tidy: Vec, + pub tidy_checks: String, + pub database: Option, + pub extra_arg: Option>, + pub thread_comments: ThreadComments, + pub no_lgtm: bool, + pub step_summary: bool, + pub file_annotations: bool, +} + +impl From<&ArgMatches> for Cli { + /// Construct a [`Cli`] instance from a [`ArgMatches`] instance (after options are parsed from CLI). + fn from(args: &ArgMatches) -> Self { + let ignore = args + .get_many::("ignore") + .unwrap() + .map(|s| s.to_owned()) + .collect::>(); + let ignore_tidy = if let Some(val) = args.get_many::("ignore-tidy") { + val.map(|s| s.to_owned()).collect::>() + } else { + vec![] + }; + let ignore_format = if let Some(val) = args.get_many::("ignore-format") { + val.map(|s| s.to_owned()).collect::>() + } else { + vec![] + }; + let extra_arg = convert_extra_arg_val(args); + + let lines_changed_only = LinesChangedOnly::from_string( + args.get_one::("lines-changed-only") + .unwrap() + .as_str(), + ); + + let thread_comments = ThreadComments::from_string( + args.get_one::("thread-comments").unwrap().as_str(), + ); + + let extensions = args + .get_many::("extensions") + .unwrap() + .map(|s| s.to_string()) + .collect::>(); + + Self { + version: args.get_one::("version").unwrap().to_owned(), + verbosity: args.get_one::("verbosity").unwrap().as_str() == "debug", + extensions, + repo_root: args.get_one::("repo-root").unwrap().to_owned(), + lines_changed_only, + files_changed_only: args.get_flag("files-changed-only"), + ignore, + style: args.get_one::("style").unwrap().to_owned(), + ignore_format, + ignore_tidy, + tidy_checks: args.get_one::("tidy-checks").unwrap().to_owned(), + database: args.get_one::("database").map(|v| v.to_owned()), + extra_arg, + no_lgtm: args.get_flag("no-lgtm"), + step_summary: args.get_flag("step-summary"), + thread_comments, + file_annotations: args.get_flag("file-annotations"), + } + } +} + +/// An enum to describe `--thread-comments` CLI option's behavior. +#[derive(PartialEq, Clone, Debug)] +pub enum ThreadComments { + /// Always post a new comment and delete any outdated ones. + On, + /// Do not post thread comments. + Off, + /// Only update existing thread comments. + /// If none exist, then post a new one. + Update, +} + +impl ThreadComments { + fn from_string(val: &str) -> ThreadComments { + match val { + "true" | "on" | "1" => ThreadComments::On, + "update" => ThreadComments::Update, + _ => ThreadComments::Off, + } + } +} + +/// A data structure to contain CLI options that relate to +/// clang-tidy or clang-format arguments. +#[derive(Debug, Clone)] +pub struct ClangParams { + pub tidy_checks: String, + pub lines_changed_only: LinesChangedOnly, + pub database: Option, + pub extra_args: Option>, + pub database_json: Option, + pub style: String, + pub clang_tidy_command: Option, + pub clang_format_command: Option, + pub tidy_filter: FileFilter, + pub format_filter: FileFilter, +} + +impl From<&Cli> for ClangParams { + /// Construct a [`ClangParams`] instance from a [`Cli`] instance. + fn from(args: &Cli) -> Self { + ClangParams { + tidy_checks: args.tidy_checks.clone(), + lines_changed_only: args.lines_changed_only.clone(), + database: args.database.clone(), + extra_args: args.extra_arg.clone(), + database_json: None, + style: args.style.clone(), + clang_tidy_command: None, + clang_format_command: None, + tidy_filter: FileFilter::new(&args.ignore_tidy, args.extensions.clone()), + format_filter: FileFilter::new(&args.ignore_format, args.extensions.clone()), + } + } +} + +/// A struct to contain CLI options that relate to +/// [`ResApiClient.post_feedback()`](fn@crate::rest_api::ResApiClient.post_feedback()). +pub struct FeedbackInput { + pub thread_comments: ThreadComments, + pub no_lgtm: bool, + pub step_summary: bool, + pub file_annotations: bool, + pub style: String, +} + +impl From<&Cli> for FeedbackInput { + /// Construct a [`FeedbackInput`] instance from a [`Cli`] instance. + fn from(args: &Cli) -> Self { + FeedbackInput { + style: args.style.clone(), + no_lgtm: args.no_lgtm, + step_summary: args.step_summary, + thread_comments: args.thread_comments.clone(), + file_annotations: args.file_annotations, + } + } +} + +impl Default for FeedbackInput { + /// Construct a [`FeedbackInput`] instance with default values. + fn default() -> Self { + FeedbackInput { + thread_comments: ThreadComments::Off, + no_lgtm: true, + step_summary: false, + file_annotations: true, + style: "llvm".to_string(), + } + } +} diff --git a/cpp-linter-lib/src/common_fs/file_filter.rs b/cpp-linter-lib/src/common_fs/file_filter.rs index 975f9fa..8b568bb 100644 --- a/cpp-linter-lib/src/common_fs/file_filter.rs +++ b/cpp-linter-lib/src/common_fs/file_filter.rs @@ -9,7 +9,7 @@ pub struct FileFilter { pub extensions: Vec, } impl FileFilter { - pub fn new(ignore: &[&str], extensions: Vec) -> Self { + pub fn new(ignore: &[String], extensions: Vec) -> Self { let (ignored, not_ignored) = Self::parse_ignore(ignore); Self { ignored, @@ -25,7 +25,7 @@ impl FileFilter { /// /// - `ignored` paths /// - `not_ignored` paths - fn parse_ignore(ignore: &[&str]) -> (Vec, Vec) { + fn parse_ignore(ignore: &[String]) -> (Vec, Vec) { let mut ignored = vec![]; let mut not_ignored = vec![]; for pattern in ignore { @@ -45,19 +45,6 @@ impl FileFilter { not_ignored.push(format!("./{pat}")); } } - - if !ignored.is_empty() { - log::info!("Ignored:"); - for pattern in &ignored { - log::info!(" {pattern}"); - } - } - if !not_ignored.is_empty() { - log::info!("Not Ignored:"); - for pattern in ¬_ignored { - log::info!(" {pattern}"); - } - } (ignored, not_ignored) } @@ -187,7 +174,7 @@ mod tests { let ignore_arg = args .get_many::("ignore") .unwrap() - .map(|s| s.as_str()) + .map(|s| s.to_owned()) .collect::>(); let file_filter = FileFilter::new(&ignore_arg, extension); println!("ignored = {:?}", file_filter.ignored); diff --git a/cpp-linter-lib/src/git.rs b/cpp-linter-lib/src/git.rs index 75b4430..683ffba 100644 --- a/cpp-linter-lib/src/git.rs +++ b/cpp-linter-lib/src/git.rs @@ -275,7 +275,7 @@ rename to /tests/demo/some source.cpp let diff_buf = RENAMED_DIFF.as_bytes(); let files = parse_diff_from_buf( diff_buf, - &FileFilter::new(&["target"], vec![String::from("cpp")]), + &FileFilter::new(&["target".to_string()], vec![String::from("cpp")]), ); assert!(!files.is_empty()); assert!(files @@ -290,7 +290,7 @@ rename to /tests/demo/some source.cpp let diff_buf = RENAMED_DIFF_WITH_CHANGES.as_bytes(); let files = parse_diff_from_buf( diff_buf, - &FileFilter::new(&["target"], vec![String::from("cpp")]), + &FileFilter::new(&["target".to_string()], vec![String::from("cpp")]), ); assert!(!files.is_empty()); } @@ -298,12 +298,13 @@ rename to /tests/demo/some source.cpp /// Used to parse the same string buffer using both libgit2 and brute force regex. /// Returns 2 vectors of [FileObj] that should be equivalent. fn setup_parsed(buf: &str, extensions: &[String]) -> (Vec, Vec) { + let ignore = ["target".to_string()]; ( parse_diff_from_buf( buf.as_bytes(), - &FileFilter::new(&["target"], extensions.to_owned()), + &FileFilter::new(&ignore, extensions.to_owned()), ), - parse_diff(buf, &FileFilter::new(&["target"], extensions.to_owned())), + parse_diff(buf, &FileFilter::new(&ignore, extensions.to_owned())), ) } @@ -399,7 +400,7 @@ mod test { patch_path, ); let rest_api_client = GithubApiClient::new(); - let file_filter = FileFilter::new(&["target"], extensions.to_owned()); + let file_filter = FileFilter::new(&["target".to_string()], extensions.to_owned()); set_current_dir(tmp).unwrap(); env::set_var("CI", "false"); // avoid use of REST API when testing in CI rest_api_client diff --git a/cpp-linter-lib/src/rest_api/github_api.rs b/cpp-linter-lib/src/rest_api/github_api.rs index 12ec1a7..e145152 100644 --- a/cpp-linter-lib/src/rest_api/github_api.rs +++ b/cpp-linter-lib/src/rest_api/github_api.rs @@ -13,13 +13,14 @@ use reqwest::Method; use serde::Deserialize; use serde_json; +// project specific modules/crates use crate::clang_tools::clang_format::tally_format_advice; use crate::clang_tools::clang_tidy::tally_tidy_advice; -// project specific modules/crates +use crate::cli::{FeedbackInput, ThreadComments}; use crate::common_fs::{FileFilter, FileObj}; use crate::git::{get_diff, open_repo, parse_diff, parse_diff_from_buf}; -use super::{FeedbackInput, RestApiClient, COMMENT_MARKER}; +use super::{RestApiClient, COMMENT_MARKER}; static USER_AGENT: &str = "Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:120.0) Gecko/20100101 Firefox/120.0"; @@ -214,7 +215,7 @@ impl RestApiClient for GithubApiClient { Some(tidy_checks_failed), ); - if user_inputs.thread_comments.as_str() != "false" { + if user_inputs.thread_comments != ThreadComments::Off { // post thread comment for PR or push event if comment.as_ref().is_some_and(|c| c.len() > 65535) || comment.is_none() { comment = Some(self.make_comment( @@ -256,7 +257,7 @@ impl RestApiClient for GithubApiClient { count, user_inputs.no_lgtm, format_checks_failed + tidy_checks_failed == 0, - user_inputs.thread_comments.as_str() == "update", + user_inputs.thread_comments == ThreadComments::Update, ) .await; } else { @@ -491,10 +492,10 @@ mod test { use super::{GithubApiClient, USER_AGENT}; use crate::{ - clang_tools::{capture_clang_tools_output, ClangParams}, - cli::LinesChangedOnly, + clang_tools::capture_clang_tools_output, + cli::{ClangParams, FeedbackInput, LinesChangedOnly}, common_fs::{FileFilter, FileObj}, - rest_api::{FeedbackInput, RestApiClient, USER_OUTREACH}, + rest_api::{RestApiClient, USER_OUTREACH}, }; // ************************** tests for GithubApiClient::make_headers() diff --git a/cpp-linter-lib/src/rest_api/mod.rs b/cpp-linter-lib/src/rest_api/mod.rs index dc2437a..a7aaa21 100644 --- a/cpp-linter-lib/src/rest_api/mod.rs +++ b/cpp-linter-lib/src/rest_api/mod.rs @@ -11,33 +11,12 @@ use reqwest::header::{HeaderMap, HeaderValue}; // project specific modules/crates pub mod github_api; +use crate::cli::FeedbackInput; use crate::common_fs::{FileFilter, FileObj}; pub static COMMENT_MARKER: &str = ""; pub static USER_OUTREACH: &str = "\n\nHave any feedback or feature suggestions? [Share it here.](https://github.com/cpp-linter/cpp-linter-action/issues)"; -/// 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 [`FeedbackInput`] 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. diff --git a/cpp-linter-lib/src/run.rs b/cpp-linter-lib/src/run.rs index 1bca001..e3e6eb4 100644 --- a/cpp-linter-lib/src/run.rs +++ b/cpp-linter-lib/src/run.rs @@ -4,7 +4,7 @@ //! `main()`. use std::env; -use std::path::{Path, PathBuf}; +use std::path::Path; use std::sync::{Arc, Mutex}; // non-std crates @@ -13,12 +13,12 @@ use log::{set_max_level, LevelFilter}; use openssl_probe; // project specific modules/crates -use crate::clang_tools::{capture_clang_tools_output, ClangParams}; -use crate::cli::{convert_extra_arg_val, get_arg_parser, LinesChangedOnly}; +use crate::clang_tools::capture_clang_tools_output; +use crate::cli::{get_arg_parser, ClangParams, Cli, FeedbackInput, LinesChangedOnly}; use crate::common_fs::FileFilter; use crate::github_api::GithubApiClient; use crate::logger::{self, end_log_group, start_log_group}; -use crate::rest_api::{FeedbackInput, RestApiClient}; +use crate::rest_api::RestApiClient; const VERSION: &str = env!("CARGO_PKG_VERSION"); @@ -49,6 +49,7 @@ pub async fn run_main(args: Vec) -> i32 { let arg_parser = get_arg_parser(); let args = arg_parser.get_matches_from(args); + let cli = Cli::from(&args); if args.subcommand_matches("version").is_some() { println!("cpp-linter v{}", VERSION); @@ -57,49 +58,40 @@ pub async fn run_main(args: Vec) -> i32 { logger::init().unwrap(); - let version = args.get_one::("version").unwrap(); - if version == "NO-VERSION" { + if cli.version == "NO-VERSION" { log::error!("The `--version` arg is used to specify which version of clang to use."); log::error!("To get the cpp-linter version, use `cpp-linter version` sub-command."); return 1; } - let root_path = args.get_one::("repo-root").unwrap(); - if root_path != &String::from(".") { - env::set_current_dir(Path::new(root_path)).unwrap(); + if cli.repo_root != "." { + env::set_current_dir(Path::new(&cli.repo_root)).unwrap(); } - let database_path = if let Some(database) = args.get_one::("database") { - if !database.is_empty() { - Some(PathBuf::from(database).canonicalize().unwrap()) - } else { - None - } - } else { - None - }; - let rest_api_client = GithubApiClient::new(); - let verbosity = args.get_one::("verbosity").unwrap().as_str() == "debug"; - set_max_level(if verbosity || rest_api_client.debug_enabled { + set_max_level(if cli.verbosity || rest_api_client.debug_enabled { LevelFilter::Debug } else { LevelFilter::Info }); log::info!("Processing event {}", rest_api_client.event_name); - let extensions = args - .get_many::("extensions") - .unwrap() - .map(|s| s.to_string()) - .collect::>(); - let ignore = args - .get_many::("ignore") - .unwrap() - .map(|s| s.as_str()) - .collect::>(); - let mut file_filter = FileFilter::new(&ignore, extensions.clone()); + let mut file_filter = FileFilter::new(&cli.ignore, cli.extensions.clone()); file_filter.parse_submodules(); + + if !file_filter.ignored.is_empty() { + log::info!("Ignored:"); + for pattern in &file_filter.ignored { + log::info!(" {pattern}"); + } + } + if !file_filter.not_ignored.is_empty() { + log::info!("Not Ignored:"); + for pattern in &file_filter.not_ignored { + log::info!(" {pattern}"); + } + } + if let Some(files) = args.get_many::>("files") { for list in files { file_filter @@ -108,19 +100,8 @@ pub async fn run_main(args: Vec) -> i32 { } } - let lines_changed_only = match args - .get_one::("lines-changed-only") - .unwrap() - .as_str() - { - "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 = if lines_changed_only != LinesChangedOnly::Off || files_changed_only { + let files = if cli.lines_changed_only != LinesChangedOnly::Off || cli.files_changed_only { // parse_diff(github_rest_api_payload) rest_api_client .get_list_of_changed_files(&file_filter) @@ -137,41 +118,9 @@ pub async fn run_main(args: Vec) -> i32 { } end_log_group(); - 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 ignore_tidy = args - .get_many::("ignore-tidy") - .unwrap() - .map(|s| s.as_str()) - .collect::>(); - let ignore_format = args - .get_many::("ignore-format") - .unwrap() - .map(|s| s.as_str()) - .collect::>(); - - let extra_args = convert_extra_arg_val(&args); - let mut clang_params = ClangParams { - tidy_checks: args.get_one::("tidy-checks").unwrap().to_string(), - lines_changed_only, - database: database_path, - extra_args, - database_json: None, - style: user_inputs.style.clone(), - clang_tidy_command: None, - clang_format_command: None, - tidy_filter: FileFilter::new(&ignore_tidy, extensions.clone()), - format_filter: FileFilter::new(&ignore_format, extensions), - }; - capture_clang_tools_output(&mut arc_files, version, &mut clang_params).await; + let mut clang_params = ClangParams::from(&cli); + let user_inputs = FeedbackInput::from(&cli); + capture_clang_tools_output(&mut arc_files, cli.version.as_str(), &mut clang_params).await; start_log_group(String::from("Posting feedback")); let checks_failed = rest_api_client.post_feedback(&arc_files, user_inputs).await; end_log_group(); diff --git a/cspell.config.yml b/cspell.config.yml index b39811a..d933cee 100644 --- a/cspell.config.yml +++ b/cspell.config.yml @@ -1,6 +1,7 @@ version: "0.2" language: en words: + - Boolish - bugprone - codecov - consts diff --git a/justfile b/justfile index faa676d..9cb67f9 100644 --- a/justfile +++ b/justfile @@ -26,7 +26,7 @@ test: [group("code coverage")] pretty-cov *args='': cargo llvm-cov report --json --output-path coverage.json - @llvm-cov-pretty coverage.json {{ args }} + llvm-cov-pretty coverage.json {{ args }} # generate and open detailed coverage report [group("code coverage")] From b2321aac5c20b00a4bbc08fff788d0478ff4a085 Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Wed, 21 Aug 2024 09:17:53 -0700 Subject: [PATCH 3/3] add tests --- cpp-linter-lib/src/cli/mod.rs | 10 +++++----- cpp-linter-lib/src/cli/structs.rs | 22 ++++++++++++++++++++++ cpp-linter-lib/src/run.rs | 19 +++++++++---------- 3 files changed, 36 insertions(+), 15 deletions(-) diff --git a/cpp-linter-lib/src/cli/mod.rs b/cpp-linter-lib/src/cli/mod.rs index e75579d..0f6e9b6 100644 --- a/cpp-linter-lib/src/cli/mod.rs +++ b/cpp-linter-lib/src/cli/mod.rs @@ -295,13 +295,13 @@ Further filtering can still be applied (see [Source options](#source-options))." ) .groups([ ArgGroup::new("Clang-tidy options") - .args(["tidy-checks", "database", "extra-arg", "ignore-tidy"]), - ArgGroup::new("Clang-format options").args(["style", "ignore-format"]), - ArgGroup::new("General options").args(["verbosity", "version"]), - ArgGroup::new("Source options").args(["extensions", "repo-root", "ignore", "lines-changed-only", "files-changed-only"]), + .args(["tidy-checks", "database", "extra-arg", "ignore-tidy"]).multiple(true), + ArgGroup::new("Clang-format options").args(["style", "ignore-format"]).multiple(true), + ArgGroup::new("General options").args(["verbosity", "version"]).multiple(true), + ArgGroup::new("Source options").args(["extensions", "repo-root", "ignore", "lines-changed-only", "files-changed-only"]).multiple(true), ArgGroup::new("Feedback options").args([ "thread-comments", "no-lgtm", "step-summary", "file-annotations" - ]), + ]).multiple(true), ]) .next_line_help(true) } diff --git a/cpp-linter-lib/src/cli/structs.rs b/cpp-linter-lib/src/cli/structs.rs index 769c36d..3ec06d3 100644 --- a/cpp-linter-lib/src/cli/structs.rs +++ b/cpp-linter-lib/src/cli/structs.rs @@ -45,6 +45,7 @@ pub struct Cli { pub no_lgtm: bool, pub step_summary: bool, pub file_annotations: bool, + pub not_ignored: Option>, } impl From<&ArgMatches> for Cli { @@ -101,6 +102,9 @@ impl From<&ArgMatches> for Cli { step_summary: args.get_flag("step-summary"), thread_comments, file_annotations: args.get_flag("file-annotations"), + not_ignored: args + .get_many::("files") + .map(|files| Vec::from_iter(files.map(|v| v.to_owned()))), } } } @@ -196,3 +200,21 @@ impl Default for FeedbackInput { } } } + +#[cfg(test)] +mod test { + use crate::cli::get_arg_parser; + + use super::Cli; + + #[test] + fn parse_positional() { + let parser = get_arg_parser(); + let args = parser.get_matches_from(["cpp-linter", "file1.c", "file2.h"]); + let cli = Cli::from(&args); + let not_ignored = cli.not_ignored.expect("failed to parse positional args"); + assert!(!not_ignored.is_empty()); + assert!(not_ignored.contains(&String::from("file1.c"))); + assert!(not_ignored.contains(&String::from("file2.h"))); + } +} diff --git a/cpp-linter-lib/src/run.rs b/cpp-linter-lib/src/run.rs index e3e6eb4..dd9b922 100644 --- a/cpp-linter-lib/src/run.rs +++ b/cpp-linter-lib/src/run.rs @@ -65,7 +65,8 @@ pub async fn run_main(args: Vec) -> i32 { } if cli.repo_root != "." { - env::set_current_dir(Path::new(&cli.repo_root)).unwrap(); + env::set_current_dir(Path::new(&cli.repo_root)) + .unwrap_or_else(|_| panic!("'{}' is inaccessible or does not exist", cli.repo_root)); } let rest_api_client = GithubApiClient::new(); @@ -78,6 +79,9 @@ pub async fn run_main(args: Vec) -> i32 { let mut file_filter = FileFilter::new(&cli.ignore, cli.extensions.clone()); file_filter.parse_submodules(); + if let Some(files) = &cli.not_ignored { + file_filter.not_ignored.extend(files.clone()); + } if !file_filter.ignored.is_empty() { log::info!("Ignored:"); @@ -92,14 +96,6 @@ pub async fn run_main(args: Vec) -> i32 { } } - if let Some(files) = args.get_many::>("files") { - for list in files { - file_filter - .not_ignored - .extend(list.iter().map(|v| v.to_string()).collect::>()); - } - } - start_log_group(String::from("Get list of specified source files")); let files = if cli.lines_changed_only != LinesChangedOnly::Off || cli.files_changed_only { // parse_diff(github_rest_api_payload) @@ -140,7 +136,10 @@ mod test { run_main(vec![ "cpp-linter".to_string(), "-l".to_string(), - "false".to_string() + "false".to_string(), + "--repo-root".to_string(), + "tests".to_string(), + "demo/demo.cpp".to_string(), ]) .await, 0