diff --git a/cpp-linter/src/clang_tools/clang_tidy.rs b/cpp-linter/src/clang_tools/clang_tidy.rs index fdf0a9a..fb11f38 100644 --- a/cpp-linter/src/clang_tools/clang_tidy.rs +++ b/cpp-linter/src/clang_tools/clang_tidy.rs @@ -518,8 +518,6 @@ mod test { database_json: None, format_filter: None, tidy_filter: None, - tidy_review: false, - format_review: false, clang_tidy_command: Some(exe_path), clang_format_command: None, repo_root: tmp_workspace.path().to_path_buf(), diff --git a/cpp-linter/src/cli/mod.rs b/cpp-linter/src/cli/mod.rs index 335a59b..3464abc 100644 --- a/cpp-linter/src/cli/mod.rs +++ b/cpp-linter/src/cli/mod.rs @@ -506,9 +506,9 @@ pub struct FeedbackOptions { ))] pub file_annotations: bool, - /// Set to `true` to enable Pull Request reviews from clang-tidy. + /// Set to `true` to enable Pull Request reviews. #[cfg_attr(feature = "bin", arg( - short = 'd', + short = 'P', long, default_value_t = false, default_missing_value = "true", @@ -516,21 +516,10 @@ pub struct FeedbackOptions { action = ArgAction::Set, value_parser = FalseyValueParser::new(), help_heading = "Feedback options", + aliases = ["format-review", "tidy-review"], + short_aliases = ['d', 'm'], ))] - pub tidy_review: bool, - - /// Set to `true` to enable Pull Request reviews from clang-format. - #[cfg_attr(feature = "bin", arg( - short = 'm', - long, - default_value_t = false, - default_missing_value = "true", - num_args = 0..=1, - action = ArgAction::Set, - value_parser = FalseyValueParser::new(), - help_heading = "Feedback options", - ))] - pub format_review: bool, + pub pr_review: bool, /// Set to `true` to prevent Pull Request reviews from /// approving or requesting changes. diff --git a/cpp-linter/src/cli/structs.rs b/cpp-linter/src/cli/structs.rs index f19073c..470e713 100644 --- a/cpp-linter/src/cli/structs.rs +++ b/cpp-linter/src/cli/structs.rs @@ -218,12 +218,6 @@ pub struct ClangParams { /// An optional [`FileFilter`] to exclude files only from clang-format analysis. pub format_filter: Option, - /// Assert this if preparing a PR review with clang-tidy advice. - pub tidy_review: bool, - - /// Assert this if preparing a PR review with clang-format advice. - pub format_review: bool, - /// The root of the repository, used to locate relative file paths in processing. /// /// A project-specific cache folder is created in this path. @@ -284,8 +278,6 @@ impl From<&Cli> for ClangParams { clang_format_command: None, tidy_filter, format_filter, - tidy_review: args.feedback_options.tidy_review, - format_review: args.feedback_options.format_review, repo_root, } } @@ -309,11 +301,8 @@ pub struct FeedbackInput { /// The clang-format style to show in file annotations. pub style: String, - /// Whether to post a PR review with clang-tidy suggestions/notes. - pub tidy_review: bool, - - /// Whether to post a PR review with clang-format suggestions. - pub format_review: bool, + /// Whether to post a PR review. + pub pr_review: bool, /// Should PR reviews be commentary? /// @@ -334,8 +323,7 @@ impl From<&Cli> for FeedbackInput { step_summary: args.feedback_options.step_summary, thread_comments: args.feedback_options.thread_comments.clone(), file_annotations: args.feedback_options.file_annotations, - tidy_review: args.feedback_options.tidy_review, - format_review: args.feedback_options.format_review, + pr_review: args.feedback_options.pr_review, passive_reviews: args.feedback_options.passive_reviews, repo_root: args.source_options.repo_root.clone(), } @@ -351,8 +339,7 @@ impl Default for FeedbackInput { step_summary: false, file_annotations: true, style: "llvm".to_string(), - tidy_review: false, - format_review: false, + pr_review: false, passive_reviews: false, repo_root: PathBuf::from("."), } diff --git a/cpp-linter/src/rest_client.rs b/cpp-linter/src/rest_client.rs index 9ff8247..05392a0 100644 --- a/cpp-linter/src/rest_client.rs +++ b/cpp-linter/src/rest_client.rs @@ -183,9 +183,7 @@ impl RestClient { }; self.client.post_thread_comment(options).await?; } - if self.client.is_pr_event() - && (feedback_inputs.tidy_review || feedback_inputs.format_review) - { + if self.client.is_pr_event() && feedback_inputs.pr_review { let summary_only = ["true", "on", "1"].contains( &env::var("CPP_LINTER_PR_REVIEW_SUMMARY_ONLY") .unwrap_or("false".to_string()) diff --git a/cpp-linter/src/run.rs b/cpp-linter/src/run.rs index 1f10424..10c624c 100644 --- a/cpp-linter/src/run.rs +++ b/cpp-linter/src/run.rs @@ -123,7 +123,7 @@ pub async fn run_main(args: Vec) -> Result<()> { FileObj::new(file_path) }) .collect(); - if is_pr && (cli.feedback_options.tidy_review || cli.feedback_options.format_review) { + if is_pr && cli.feedback_options.pr_review { let changed_files = rest_api_client .get_list_of_changed_files( &file_filter, @@ -152,7 +152,7 @@ pub async fn run_main(args: Vec) -> Result<()> { } rest_api_client.end_log_group("Get list of specified source files"); - let mut clang_params = ClangParams::from(&cli); + let clang_params = ClangParams::from(&cli); // mkdir -p .cpp-linter-cache/ let cache_dir = clang_params.repo_root.join(ClangParams::CACHE_DIR); std::fs::create_dir_all(&cache_dir) @@ -163,8 +163,6 @@ pub async fn run_main(args: Vec) -> Result<()> { "# Automatically created by cpp-linter\n*\n", ) .with_context(|| "Failed to write .cpp-linter-cache/.gitignore file")?; - clang_params.format_review &= is_pr; - clang_params.tidy_review &= is_pr; let user_inputs = FeedbackInput::from(&cli); let clang_versions = capture_clang_tools_output( &arc_files, diff --git a/cpp-linter/tests/reviews.rs b/cpp-linter/tests/reviews.rs index 00d3001..e4df77e 100644 --- a/cpp-linter/tests/reviews.rs +++ b/cpp-linter/tests/reviews.rs @@ -23,8 +23,7 @@ const REMAINING_RATE_LIMIT_HEADER: &str = "x-ratelimit-remaining"; struct TestParams { pub lines_changed_only: LinesChangedOnly, - pub tidy_review: bool, - pub format_review: bool, + pub pr_review: bool, pub passive_reviews: bool, pub no_lgtm: bool, pub force_lgtm: bool, @@ -42,8 +41,7 @@ impl Default for TestParams { fn default() -> Self { Self { lines_changed_only: LinesChangedOnly::On, - tidy_review: true, - format_review: true, + pr_review: true, passive_reviews: false, no_lgtm: true, force_lgtm: false, @@ -175,9 +173,7 @@ async fn setup(lib_root: &Path, tmp_dir: &TempDir, test_params: &TestParams) { "REQUEST_CHANGES" }; let tool_summary = { - if !(test_params.format_review || test_params.tidy_review) - || test_params.lines_changed_only == LinesChangedOnly::On - { + if !test_params.pr_review || test_params.lines_changed_only == LinesChangedOnly::On { // results can be non-deterministic because different clang-versions output different diagnostic/changes ".*" // match anything } else if test_params.force_lgtm { @@ -226,8 +222,7 @@ async fn setup(lib_root: &Path, tmp_dir: &TempDir, test_params: &TestParams) { format!("-l={}", test_params.lines_changed_only), format!("--ignore-tidy={}", tidy_ignore), format!("--ignore-format={}", format_ignore), - format!("--tidy-review={}", test_params.tidy_review), - format!("--format-review={}", test_params.format_review), + format!("--pr-review={}", test_params.pr_review), format!("--passive-reviews={}", test_params.passive_reviews), format!("--no-lgtm={}", test_params.no_lgtm), "-p=build".to_string(), @@ -235,15 +230,14 @@ async fn setup(lib_root: &Path, tmp_dir: &TempDir, test_params: &TestParams) { format!("--repo-root={}", tmp_dir.path().to_str().unwrap()), ]; if test_params.force_lgtm { - if test_params.tidy_review { + if test_params.pr_review { // only use a check that doesn't trigger concern on test assets args.push("--tidy-checks=-*,bugprone-infinite-loop".to_string()); - } - if test_params.format_review { // explicitly disable formatting using `DisableFormat: true` args.push("--style={DisableFormat: true}".to_string()); } } else { + // use default --tidy-checks value args.push("--style=file".to_string()); // use .clang-format file } match run_main(args).await { @@ -279,26 +273,6 @@ async fn all_lines() { .await; } -#[tokio::test] -async fn all_lines_tidy_only() { - test_review(&TestParams { - lines_changed_only: LinesChangedOnly::Off, - format_review: false, - ..Default::default() - }) - .await; -} - -#[tokio::test] -async fn all_lines_format_only() { - test_review(&TestParams { - lines_changed_only: LinesChangedOnly::Off, - tidy_review: false, - ..Default::default() - }) - .await; -} - #[tokio::test] async fn changed_lines() { test_review(&TestParams::default()).await;