diff --git a/Cargo.toml b/Cargo.toml index 80c0b4fe..7227ea3c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -13,6 +13,7 @@ authors = [ description = "Run clang-format and clang-tidy on a batch of files." homepage = "https://cpp-linter.github.io/cpp-linter-rs" license = "MIT" +edition = "2024" [profile.release] lto = true diff --git a/bindings/node/Cargo.toml b/bindings/node/Cargo.toml index 10e8030d..146dc08c 100644 --- a/bindings/node/Cargo.toml +++ b/bindings/node/Cargo.toml @@ -1,11 +1,11 @@ [package] name = "cpp-linter-js" -edition = "2021" readme = "README.md" keywords = ["clang-tidy", "clang-format", "linter"] categories = ["command-line-utilities", "development-tools", "filesystem"] repository = "https://github.com/cpp-linter/cpp-linter-rs" version.workspace = true +edition.workspace = true authors.workspace = true description.workspace = true homepage.workspace = true diff --git a/bindings/python/Cargo.toml b/bindings/python/Cargo.toml index 4c03b2f8..f16213c0 100644 --- a/bindings/python/Cargo.toml +++ b/bindings/python/Cargo.toml @@ -1,9 +1,9 @@ [package] name = "cpp-linter-py" -edition = "2021" readme = "README.md" repository = "https://github.com/cpp-linter/cpp-linter-rs/tree/main/bindings/python" version.workspace = true +edition.workspace = true authors.workspace = true description.workspace = true homepage.workspace = true diff --git a/cpp-linter/Cargo.toml b/cpp-linter/Cargo.toml index 527650c3..96c035cd 100644 --- a/cpp-linter/Cargo.toml +++ b/cpp-linter/Cargo.toml @@ -1,11 +1,11 @@ [package] name = "cpp-linter" -edition = "2021" readme = "README.md" keywords = ["clang-tidy", "clang-format", "linter"] categories = ["command-line-utilities", "development-tools", "filesystem"] repository = "https://github.com/cpp-linter/cpp-linter-rs" version.workspace = true +edition.workspace = true authors.workspace = true description.workspace = true homepage.workspace = true diff --git a/cpp-linter/src/clang_tools/clang_format.rs b/cpp-linter/src/clang_tools/clang_format.rs index 40689d61..dce92bbc 100644 --- a/cpp-linter/src/clang_tools/clang_format.rs +++ b/cpp-linter/src/clang_tools/clang_format.rs @@ -15,7 +15,7 @@ use serde::Deserialize; use super::MakeSuggestions; use crate::{ cli::ClangParams, - common_fs::{get_line_count_from_offset, FileObj}, + common_fs::{FileObj, get_line_count_from_offset}, }; #[derive(Debug, Clone, Deserialize, PartialEq, Eq)] @@ -71,10 +71,10 @@ pub fn tally_format_advice(files: &[Arc>]) -> u64 { let mut total = 0; for file in files { let file = file.lock().unwrap(); - if let Some(advice) = &file.format_advice { - if !advice.replacements.is_empty() { - total += 1; - } + if let Some(advice) = &file.format_advice + && !advice.replacements.is_empty() + { + total += 1; } } total @@ -187,7 +187,7 @@ pub fn run_clang_format( #[cfg(test)] mod tests { - use super::{summarize_style, FormatAdvice, Replacement}; + use super::{FormatAdvice, Replacement, summarize_style}; #[test] fn parse_blank_xml() { diff --git a/cpp-linter/src/clang_tools/clang_tidy.rs b/cpp-linter/src/clang_tools/clang_tidy.rs index 1adf9bde..6b3fe76a 100644 --- a/cpp-linter/src/clang_tools/clang_tidy.rs +++ b/cpp-linter/src/clang_tools/clang_tidy.rs @@ -18,7 +18,7 @@ use serde::Deserialize; use super::MakeSuggestions; use crate::{ cli::{ClangParams, LinesChangedOnly}, - common_fs::{normalize_path, FileObj}, + common_fs::{FileObj, normalize_path}, }; /// Used to deserialize a json compilation database's translation unit. @@ -205,21 +205,19 @@ fn parse_tidy_output( found_fix = false; } else if let Some(capture) = fixed_note.captures(line) { let fixed_line = capture[1].parse()?; - if let Some(note) = &mut notification { - if !note.fixed_lines.contains(&fixed_line) { - note.fixed_lines.push(fixed_line); - } + if let Some(note) = &mut notification + && !note.fixed_lines.contains(&fixed_line) + { + note.fixed_lines.push(fixed_line); } // Suspend capturing subsequent lines as suggestions until // a new notification is constructed. If we found a note about applied fixes, // then the lines of suggestions for that notification have already been parsed. found_fix = true; - } else if !found_fix { - if let Some(note) = &mut notification { - // append lines of code that are part of - // the previous line's notification - note.suggestion.push(line.to_string()); - } + } else if !found_fix && let Some(note) = &mut notification { + // append lines of code that are part of + // the previous line's notification + note.suggestion.push(line.to_string()); } } if let Some(note) = notification { @@ -358,7 +356,7 @@ mod test { common_fs::FileObj, }; - use super::{run_clang_tidy, TidyNotification, NOTE_HEADER}; + use super::{NOTE_HEADER, TidyNotification, run_clang_tidy}; #[test] fn clang_diagnostic_link() { diff --git a/cpp-linter/src/clang_tools/mod.rs b/cpp-linter/src/clang_tools/mod.rs index 7a1cc233..1879ff65 100644 --- a/cpp-linter/src/clang_tools/mod.rs +++ b/cpp-linter/src/clang_tools/mod.rs @@ -11,7 +11,7 @@ use std::{ }; // non-std crates -use anyhow::{anyhow, Context, Result}; +use anyhow::{Context, Result, anyhow}; use git2::{DiffOptions, Patch}; use regex::Regex; use semver::Version; @@ -22,12 +22,12 @@ use which::{which, which_in}; use super::common_fs::FileObj; use crate::{ cli::{ClangParams, RequestedVersion}, - rest_api::{RestApiClient, COMMENT_MARKER, USER_OUTREACH}, + rest_api::{COMMENT_MARKER, RestApiClient, USER_OUTREACH}, }; pub mod clang_format; use clang_format::run_clang_format; pub mod clang_tidy; -use clang_tidy::{run_clang_tidy, CompilationUnit}; +use clang_tidy::{CompilationUnit, run_clang_tidy}; #[derive(Debug)] pub enum ClangTool { @@ -229,14 +229,14 @@ pub async fn capture_clang_tools_output( } // parse database (if provided) to match filenames when parsing clang-tidy's stdout - if let Some(db_path) = &clang_params.database { - if let Ok(db_str) = fs::read(db_path.join("compile_commands.json")) { - clang_params.database_json = Some( - // A compilation database should be UTF-8 encoded, but file paths are not; use lossy conversion. - serde_json::from_str::>(&String::from_utf8_lossy(&db_str)) - .with_context(|| "Failed to parse compile_commands.json")?, - ) - } + if let Some(db_path) = &clang_params.database + && let Ok(db_str) = fs::read(db_path.join("compile_commands.json")) + { + clang_params.database_json = Some( + // A compilation database should be UTF-8 encoded, but file paths are not; use lossy conversion. + serde_json::from_str::>(&String::from_utf8_lossy(&db_str)) + .with_context(|| "Failed to parse compile_commands.json")?, + ) }; let mut executors = JoinSet::new(); @@ -503,24 +503,26 @@ mod tests { let req_version = RequestedVersion::from_str(requirement).unwrap(); let tool_exe = CLANG_FORMAT.get_exe_path(&req_version); println!("tool_exe: {:?}", tool_exe); - assert!(tool_exe.is_ok_and(|val| val - .file_name() - .unwrap() - .to_string_lossy() - .to_string() - .contains(CLANG_FORMAT.as_str()))); + assert!(tool_exe.is_ok_and(|val| { + val.file_name() + .unwrap() + .to_string_lossy() + .to_string() + .contains(CLANG_FORMAT.as_str()) + })); } #[test] fn get_exe_by_default() { let tool_exe = CLANG_FORMAT.get_exe_path(&RequestedVersion::from_str("").unwrap()); println!("tool_exe: {:?}", tool_exe); - assert!(tool_exe.is_ok_and(|val| val - .file_name() - .unwrap() - .to_string_lossy() - .to_string() - .contains(CLANG_FORMAT.as_str()))); + assert!(tool_exe.is_ok_and(|val| { + val.file_name() + .unwrap() + .to_string_lossy() + .to_string() + .contains(CLANG_FORMAT.as_str()) + })); } #[test] @@ -531,12 +533,13 @@ mod tests { println!("binary exe path: {bin_path}"); let tool_exe = CLANG_FORMAT.get_exe_path(&RequestedVersion::from_str(bin_path).unwrap()); println!("tool_exe: {:?}", tool_exe); - assert!(tool_exe.is_ok_and(|val| val - .file_name() - .unwrap() - .to_string_lossy() - .to_string() - .contains(TOOL_NAME))); + assert!(tool_exe.is_ok_and(|val| { + val.file_name() + .unwrap() + .to_string_lossy() + .to_string() + .contains(TOOL_NAME) + })); } #[test] diff --git a/cpp-linter/src/cli/mod.rs b/cpp-linter/src/cli/mod.rs index 211d9849..9e8308a2 100644 --- a/cpp-linter/src/cli/mod.rs +++ b/cpp-linter/src/cli/mod.rs @@ -3,8 +3,9 @@ use std::{path::PathBuf, str::FromStr}; // non-std crates use clap::{ + ArgAction, Args, Parser, Subcommand, ValueEnum, builder::{FalseyValueParser, NonEmptyStringValueParser}, - value_parser, ArgAction, Args, Parser, Subcommand, ValueEnum, + value_parser, }; mod structs; @@ -453,7 +454,7 @@ pub fn convert_extra_arg_val(args: &[String]) -> Vec { #[cfg(test)] mod test { - use super::{convert_extra_arg_val, Cli}; + use super::{Cli, convert_extra_arg_val}; use clap::Parser; #[test] diff --git a/cpp-linter/src/cli/structs.rs b/cpp-linter/src/cli/structs.rs index f4cf3d27..84f683d0 100644 --- a/cpp-linter/src/cli/structs.rs +++ b/cpp-linter/src/cli/structs.rs @@ -1,13 +1,13 @@ use std::{fmt::Display, path::PathBuf, str::FromStr}; -use anyhow::{anyhow, Error}; -use clap::{builder::PossibleValue, ValueEnum}; +use anyhow::{Error, anyhow}; +use clap::{ValueEnum, builder::PossibleValue}; use semver::VersionReq; use super::Cli; use crate::{ clang_tools::clang_tidy::CompilationUnit, - common_fs::{normalize_path, FileFilter}, + common_fs::{FileFilter, normalize_path}, }; #[derive(Debug, Clone, PartialEq, Eq, Default)] diff --git a/cpp-linter/src/common_fs/file_filter.rs b/cpp-linter/src/common_fs/file_filter.rs index 3760013b..e474dc79 100644 --- a/cpp-linter/src/common_fs/file_filter.rs +++ b/cpp-linter/src/common_fs/file_filter.rs @@ -1,4 +1,4 @@ -use anyhow::{anyhow, Context, Result}; +use anyhow::{Context, Result, anyhow}; use fast_glob::glob_match; use std::{ fs, @@ -257,14 +257,16 @@ mod tests { let files = file_filter.list_source_files(".").unwrap(); assert!(!files.is_empty()); for file in files { - assert!(extensions.contains( - &file - .name - .extension() - .unwrap_or_default() - .to_string_lossy() - .to_string() - )); + assert!( + extensions.contains( + &file + .name + .extension() + .unwrap_or_default() + .to_string_lossy() + .to_string() + ) + ); } } } diff --git a/cpp-linter/src/common_fs/mod.rs b/cpp-linter/src/common_fs/mod.rs index c07854bf..199dd3a4 100644 --- a/cpp-linter/src/common_fs/mod.rs +++ b/cpp-linter/src/common_fs/mod.rs @@ -9,7 +9,7 @@ use anyhow::{Context, Result}; use crate::clang_tools::clang_format::FormatAdvice; use crate::clang_tools::clang_tidy::TidyAdvice; -use crate::clang_tools::{make_patch, MakeSuggestions, ReviewComments, Suggestion}; +use crate::clang_tools::{MakeSuggestions, ReviewComments, Suggestion, make_patch}; use crate::cli::LinesChangedOnly; mod file_filter; pub use file_filter::FileFilter; @@ -147,11 +147,11 @@ impl FileObj { fs::read(&self.name).with_context(|| "Failed to read original contents of file")?; let file_name = self.name.to_str().unwrap_or_default().replace("\\", "/"); let file_path = Path::new(&file_name); - if let Some(advice) = &self.format_advice { - if let Some(patched) = &advice.patched { - let mut patch = make_patch(file_path, patched, &original_content)?; - advice.get_suggestions(review_comments, self, &mut patch, summary_only)?; - } + if let Some(advice) = &self.format_advice + && let Some(patched) = &advice.patched + { + let mut patch = make_patch(file_path, patched, &original_content)?; + advice.get_suggestions(review_comments, self, &mut patch, summary_only)?; } if let Some(advice) = &self.tidy_advice { @@ -266,7 +266,7 @@ mod test { use std::path::PathBuf; use std::{env::current_dir, fs}; - use super::{get_line_count_from_offset, normalize_path, FileObj}; + use super::{FileObj, get_line_count_from_offset, normalize_path}; use crate::cli::LinesChangedOnly; // *********************** tests for normalized paths diff --git a/cpp-linter/src/git.rs b/cpp-linter/src/git.rs index 8d2d93f3..3ffb5d7e 100644 --- a/cpp-linter/src/git.rs +++ b/cpp-linter/src/git.rs @@ -180,10 +180,10 @@ mod brute_force_parse_diff { if let Some(captures) = diff_file_name.captures(front_matter) { return Some(captures.get(1).unwrap().as_str()); } - if front_matter.trim_start().starts_with("similarity") { - if let Some(captures) = diff_renamed_file.captures(front_matter) { - return Some(captures.get(1).unwrap().as_str()); - } + if front_matter.trim_start().starts_with("similarity") + && let Some(captures) = diff_renamed_file.captures(front_matter) + { + return Some(captures.get(1).unwrap().as_str()); } if !diff_binary_file.is_match(front_matter) { log::warn!("Unrecognized diff starting with:\n{}", front_matter); @@ -311,11 +311,13 @@ rename to /tests/demo/some source.c &LinesChangedOnly::Off, ); assert!(!files.is_empty()); - assert!(files - .first() - .unwrap() - .name - .ends_with("tests/demo/some source.c")); + assert!( + files + .first() + .unwrap() + .name + .ends_with("tests/demo/some source.c") + ); } #[test] @@ -415,12 +417,12 @@ mod test { } } - use tempfile::{tempdir, TempDir}; + use tempfile::{TempDir, tempdir}; use crate::{ cli::LinesChangedOnly, common_fs::FileFilter, - rest_api::{github::GithubApiClient, RestApiClient}, + rest_api::{RestApiClient, github::GithubApiClient}, }; fn get_temp_dir() -> TempDir { @@ -445,7 +447,10 @@ mod test { let rest_api_client = GithubApiClient::new(); 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 + // avoid use of REST API when testing in CI + unsafe { + env::set_var("CI", "false"); + } rest_api_client .unwrap() .get_list_of_changed_files(&file_filter, &LinesChangedOnly::Off) diff --git a/cpp-linter/src/logger.rs b/cpp-linter/src/logger.rs index 49a5c7d4..c572c80d 100644 --- a/cpp-linter/src/logger.rs +++ b/cpp-linter/src/logger.rs @@ -2,10 +2,10 @@ use std::{ env, - io::{stdout, Write}, + io::{Write, stdout}, }; -use colored::{control::set_override, Colorize}; +use colored::{Colorize, control::set_override}; use log::{Level, LevelFilter, Log, Metadata, Record}; #[derive(Default)] @@ -93,11 +93,13 @@ pub fn try_init() { mod test { use std::env; - use super::{try_init, SimpleLogger}; + use super::{SimpleLogger, try_init}; #[test] fn trace_log() { - env::set_var("CPP_LINTER_COLOR", "true"); + unsafe { + env::set_var("CPP_LINTER_COLOR", "true"); + } try_init(); assert!(SimpleLogger::level_color(&log::Level::Trace).contains("TRACE")); log::set_max_level(log::LevelFilter::Trace); diff --git a/cpp-linter/src/rest_api/github/mod.rs b/cpp-linter/src/rest_api/github/mod.rs index 756c81af..e1b20ea7 100644 --- a/cpp-linter/src/rest_api/github/mod.rs +++ b/cpp-linter/src/rest_api/github/mod.rs @@ -11,15 +11,15 @@ use std::sync::{Arc, Mutex}; // non-std crates use anyhow::{Context, Result}; use reqwest::{ - header::{HeaderMap, HeaderValue, AUTHORIZATION}, Client, Method, Url, + header::{AUTHORIZATION, HeaderMap, HeaderValue}, }; // project specific modules/crates -use super::{send_api_request, RestApiClient, RestApiRateLimitHeaders}; +use super::{RestApiClient, RestApiRateLimitHeaders, send_api_request}; +use crate::clang_tools::ClangVersions; use crate::clang_tools::clang_format::tally_format_advice; use crate::clang_tools::clang_tidy::tally_tidy_advice; -use crate::clang_tools::ClangVersions; use crate::cli::{FeedbackInput, LinesChangedOnly, ThreadComments}; use crate::common_fs::{FileFilter, FileObj}; use crate::git::{get_diff, open_repo, parse_diff, parse_diff_from_buf}; @@ -170,9 +170,9 @@ impl RestApiClient for GithubApiClient { } } else { // get diff from libgit2 API - let repo = open_repo(".").with_context(|| { - "Please ensure the repository is checked out before running cpp-linter." - })?; + let repo = open_repo(".").with_context( + || "Please ensure the repository is checked out before running cpp-linter.", + )?; let list = parse_diff(&get_diff(&repo)?, file_filter, lines_changed_only); Ok(list) } @@ -261,14 +261,14 @@ mod test { }; use regex::Regex; - use tempfile::{tempdir, NamedTempFile}; + use tempfile::{NamedTempFile, tempdir}; use super::GithubApiClient; use crate::{ clang_tools::{ + ClangVersions, clang_format::{FormatAdvice, Replacement}, clang_tidy::{TidyAdvice, TidyNotification}, - ClangVersions, }, cli::{FeedbackInput, LinesChangedOnly}, common_fs::{FileFilter, FileObj}, @@ -326,23 +326,25 @@ mod test { ..Default::default() }; let mut step_summary_path = NamedTempFile::new_in(tmp_dir.path()).unwrap(); - env::set_var( - "GITHUB_STEP_SUMMARY", - if fail_summary { - Path::new("not-a-file.txt") - } else { - step_summary_path.path() - }, - ); let mut gh_out_path = NamedTempFile::new_in(tmp_dir.path()).unwrap(); - env::set_var( - "GITHUB_OUTPUT", - if fail_gh_out { - Path::new("not-a-file.txt") - } else { - gh_out_path.path() - }, - ); + unsafe { + env::set_var( + "GITHUB_STEP_SUMMARY", + if fail_summary { + Path::new("not-a-file.txt") + } else { + step_summary_path.path() + }, + ); + env::set_var( + "GITHUB_OUTPUT", + if fail_gh_out { + Path::new("not-a-file.txt") + } else { + gh_out_path.path() + }, + ); + } let clang_versions = ClangVersions { format_version: Some("x.y.z".to_string()), tidy_version: Some("x.y.z".to_string()), @@ -387,7 +389,9 @@ mod test { #[tokio::test] async fn check_comment_lgtm() { - env::set_var("ACTIONS_STEP_DEBUG", "true"); + unsafe { + env::set_var("ACTIONS_STEP_DEBUG", "true"); + } let (comment, gh_out) = create_comment(true, false, false).await; assert!(comment.contains(":heavy_check_mark:\nNo problems need attention.")); assert_eq!( @@ -398,7 +402,9 @@ mod test { #[tokio::test] async fn fail_gh_output() { - env::set_var("ACTIONS_STEP_DEBUG", "true"); + unsafe { + env::set_var("ACTIONS_STEP_DEBUG", "true"); + } let (comment, gh_out) = create_comment(true, true, false).await; assert!(&comment.contains(":heavy_check_mark:\nNo problems need attention.")); assert!(gh_out.is_empty()); @@ -406,7 +412,9 @@ mod test { #[tokio::test] async fn fail_gh_summary() { - env::set_var("ACTIONS_STEP_DEBUG", "true"); + unsafe { + env::set_var("ACTIONS_STEP_DEBUG", "true"); + } let (comment, gh_out) = create_comment(true, false, true).await; assert!(comment.is_empty()); assert_eq!( @@ -417,7 +425,9 @@ mod test { #[tokio::test] async fn fail_get_local_diff() { - env::set_var("CI", "false"); + unsafe { + env::set_var("CI", "false"); + } let tmp_dir = tempdir().unwrap(); env::set_current_dir(tmp_dir.path()).unwrap(); let rest_client = GithubApiClient::new().unwrap(); diff --git a/cpp-linter/src/rest_api/github/specific_api.rs b/cpp-linter/src/rest_api/github/specific_api.rs index 67b0747f..4f7a17f7 100644 --- a/cpp-linter/src/rest_api/github/specific_api.rs +++ b/cpp-linter/src/rest_api/github/specific_api.rs @@ -9,23 +9,23 @@ use std::{ sync::{Arc, Mutex}, }; -use anyhow::{anyhow, Context, Result}; +use anyhow::{Context, Result, anyhow}; use reqwest::{Client, Method, Url}; use crate::{ - clang_tools::{clang_format::summarize_style, ClangVersions, ReviewComments}, + clang_tools::{ClangVersions, ReviewComments, clang_format::summarize_style}, cli::{FeedbackInput, LinesChangedOnly}, common_fs::{FileFilter, FileObj}, git::parse_diff_from_buf, - rest_api::{send_api_request, RestApiRateLimitHeaders, COMMENT_MARKER, USER_AGENT}, + rest_api::{COMMENT_MARKER, RestApiRateLimitHeaders, USER_AGENT, send_api_request}, }; use super::{ + GithubApiClient, RestApiClient, serde_structs::{ - FullReview, GithubChangedFile, PullRequestInfo, PushEventFiles, ReviewComment, - ReviewDiffComment, ThreadComment, REVIEW_DISMISSAL, + FullReview, GithubChangedFile, PullRequestInfo, PushEventFiles, REVIEW_DISMISSAL, + ReviewComment, ReviewDiffComment, ThreadComment, }, - GithubApiClient, RestApiClient, }; impl GithubApiClient { @@ -100,15 +100,15 @@ impl GithubApiClient { url = Self::try_next_page(response.headers()); let files_list = if self.event_name != "pull_request" { let json_value: PushEventFiles = serde_json::from_str(&response.text().await?) - .with_context(|| { - "Failed to deserialize list of changed files from json response" - })?; + .with_context( + || "Failed to deserialize list of changed files from json response", + )?; json_value.files } else { serde_json::from_str::>(&response.text().await?) - .with_context(|| { - "Failed to deserialize list of file changes from Pull Request event." - })? + .with_context( + || "Failed to deserialize list of file changes from Pull Request event.", + )? }; for file in files_list { let ext = Path::new(&file.filename).extension().unwrap_or_default(); @@ -173,10 +173,14 @@ impl GithubApiClient { // post annotation if any applicable lines were formatted if !lines.is_empty() { println!( - "::notice file={name},title=Run clang-format on {name}::File {name} does not conform to {style_guide} style guidelines. (lines {line_set})", - name = &file.name.to_string_lossy().replace('\\', "/"), - line_set = lines.iter().map(|val| val.to_string()).collect::>().join(","), - ); + "::notice file={name},title=Run clang-format on {name}::File {name} does not conform to {style_guide} style guidelines. (lines {line_set})", + name = &file.name.to_string_lossy().replace('\\', "/"), + line_set = lines + .iter() + .map(|val| val.to_string()) + .collect::>() + .join(","), + ); } } // end format_advice iterations @@ -188,7 +192,11 @@ impl GithubApiClient { if note.filename == file.name.to_string_lossy().replace('\\', "/") { println!( "::{severity} file={file},line={line},title={file}:{line}:{cols} [{diag}]::{info}", - severity = if note.severity == *"note" { "notice".to_string() } else {note.severity.clone()}, + severity = if note.severity == *"note" { + "notice".to_string() + } else { + note.severity.clone() + }, file = note.filename, line = note.line, cols = note.cols, @@ -485,45 +493,42 @@ impl GithubApiClient { } Ok(payload) => { for review in payload { - if let Some(body) = &review.body { - if body.starts_with(COMMENT_MARKER) - && !(["PENDING", "DISMISSED"] - .contains(&review.state.as_str())) + if let Some(body) = &review.body + && body.starts_with(COMMENT_MARKER) + && !(["PENDING", "DISMISSED"].contains(&review.state.as_str())) + { + // dismiss outdated review + if let Ok(dismiss_url) = url + .join(format!("reviews/{}/dismissals", review.id).as_str()) + && let Ok(req) = Self::make_api_request( + &self.client, + dismiss_url, + Method::PUT, + Some(REVIEW_DISMISSAL.to_string()), + None, + ) { - // dismiss outdated review - if let Ok(dismiss_url) = url.join( - format!("reviews/{}/dismissals", review.id).as_str(), - ) { - if let Ok(req) = Self::make_api_request( - &self.client, - dismiss_url, - Method::PUT, - Some(REVIEW_DISMISSAL.to_string()), - None, - ) { - match send_api_request( - &self.client, - req, - &self.rate_limit_headers, - ) - .await - { - Ok(result) => { - if !result.status().is_success() { - Self::log_response( - result, - "Failed to dismiss outdated review", - ) - .await; - } - } - Err(e) => { - log::error!( - "Failed to dismiss outdated review: {e:}" - ); - } + match send_api_request( + &self.client, + req, + &self.rate_limit_headers, + ) + .await + { + Ok(result) => { + if !result.status().is_success() { + Self::log_response( + result, + "Failed to dismiss outdated review", + ) + .await; } } + Err(e) => { + log::error!( + "Failed to dismiss outdated review: {e:}" + ); + } } } } diff --git a/cpp-linter/src/rest_api/mod.rs b/cpp-linter/src/rest_api/mod.rs index 3d60fba9..4eb5b4e3 100644 --- a/cpp-linter/src/rest_api/mod.rs +++ b/cpp-linter/src/rest_api/mod.rs @@ -10,7 +10,7 @@ use std::sync::{Arc, Mutex}; use std::time::Duration; // non-std crates -use anyhow::{anyhow, Error, Result}; +use anyhow::{Error, Result, anyhow}; use chrono::DateTime; use reqwest::header::{HeaderMap, HeaderValue}; use reqwest::{Client, IntoUrl, Method, Request, Response, Url}; @@ -180,21 +180,21 @@ pub trait RestApiClient { /// /// Returns [`None`] if current response is the last page. fn try_next_page(headers: &HeaderMap) -> Option { - if let Some(links) = headers.get("link") { - if let Ok(pg_str) = links.to_str() { - let pages = pg_str.split(", "); - for page in pages { - if page.ends_with("; rel=\"next\"") { - if let Some(link) = page.split_once(">;") { - let url = link.0.trim_start_matches("<").to_string(); - if let Ok(next) = Url::parse(&url) { - return Some(next); - } else { - log::debug!("Failed to parse next page link from response header"); - } + if let Some(links) = headers.get("link") + && let Ok(pg_str) = links.to_str() + { + let pages = pg_str.split(", "); + for page in pages { + if page.ends_with("; rel=\"next\"") { + if let Some(link) = page.split_once(">;") { + let url = link.0.trim_start_matches("<").to_string(); + if let Ok(next) = Url::parse(&url) { + return Some(next); } else { - log::debug!("Response header link for pagination is malformed"); + log::debug!("Failed to parse next page link from response header"); } + } else { + log::debug!("Response header link for pagination is malformed"); } } } @@ -244,8 +244,8 @@ pub async fn send_api_request( requests_remaining = Some(value); } else { log::debug!( - "Failed to parse i64 from remaining attempts about rate limit: {count}" - ); + "Failed to parse i64 from remaining attempts about rate limit: {count}" + ); } } } else { @@ -283,8 +283,8 @@ pub async fn send_api_request( tokio::time::sleep(interval).await; } else { log::debug!( - "Failed to parse u64 from retry interval about rate limit: {retry_str}" - ); + "Failed to parse u64 from retry interval about rate limit: {retry_str}" + ); } } continue; @@ -314,13 +314,14 @@ fn make_format_comment( *remaining_length -= opener.len() as u64 + closer.len() as u64; for file in files { let file = file.lock().unwrap(); - if let Some(format_advice) = &file.format_advice { - if !format_advice.replacements.is_empty() && *remaining_length > 0 { - let note = format!("- {}\n", file.name.to_string_lossy().replace('\\', "/")); - if (note.len() as u64) < *remaining_length { - format_comment.push_str(¬e.to_string()); - *remaining_length -= note.len() as u64; - } + if let Some(format_advice) = &file.format_advice + && !format_advice.replacements.is_empty() + && *remaining_length > 0 + { + let note = format!("- {}\n", file.name.to_string_lossy().replace('\\', "/")); + if (note.len() as u64) < *remaining_length { + format_comment.push_str(¬e.to_string()); + *remaining_length -= note.len() as u64; } } } @@ -384,13 +385,13 @@ fn make_tidy_comment( mod test { use std::sync::{Arc, Mutex}; - use anyhow::{anyhow, Result}; + use anyhow::{Result, anyhow}; use chrono::Utc; use mockito::{Matcher, Server}; use reqwest::Method; use reqwest::{ - header::{HeaderMap, HeaderValue}, Client, + header::{HeaderMap, HeaderValue}, }; use crate::cli::LinesChangedOnly; @@ -401,7 +402,7 @@ mod test { logger, }; - use super::{send_api_request, RestApiClient, RestApiRateLimitHeaders}; + use super::{RestApiClient, RestApiRateLimitHeaders, send_api_request}; /// A dummy struct to impl RestApiClient #[derive(Default)] @@ -453,14 +454,18 @@ mod test { let dummy = TestClient::default(); dummy.start_log_group("Dummy test".to_string()); assert_eq!(dummy.set_exit_code(1, None, None), 0); - assert!(dummy - .get_list_of_changed_files(&FileFilter::new(&[], vec![]), &LinesChangedOnly::Off) - .await - .is_err()); - assert!(dummy - .post_feedback(&[], FeedbackInput::default(), ClangVersions::default()) - .await - .is_err()); + assert!( + dummy + .get_list_of_changed_files(&FileFilter::new(&[], vec![]), &LinesChangedOnly::Off) + .await + .is_err() + ); + assert!( + dummy + .post_feedback(&[], FeedbackInput::default(), ClangVersions::default()) + .await + .is_err() + ); dummy.end_log_group(); } @@ -469,9 +474,11 @@ mod test { #[test] fn bad_link_header() { let mut headers = HeaderMap::with_capacity(1); - assert!(headers - .insert("link", HeaderValue::from_str("; rel=\"next\"").unwrap()) - .is_none()); + assert!( + headers + .insert("link", HeaderValue::from_str("; rel=\"next\"").unwrap()) + .is_none() + ); logger::try_init(); log::set_max_level(log::LevelFilter::Debug); let result = TestClient::try_next_page(&headers); @@ -481,12 +488,14 @@ mod test { #[test] fn bad_link_domain() { let mut headers = HeaderMap::with_capacity(1); - assert!(headers - .insert( - "link", - HeaderValue::from_str("; rel=\"next\"").unwrap() - ) - .is_none()); + assert!( + headers + .insert( + "link", + HeaderValue::from_str("; rel=\"next\"").unwrap() + ) + .is_none() + ); logger::try_init(); log::set_max_level(log::LevelFilter::Debug); let result = TestClient::try_next_page(&headers); diff --git a/cpp-linter/src/run.rs b/cpp-linter/src/run.rs index c22e862d..88ef076d 100644 --- a/cpp-linter/src/run.rs +++ b/cpp-linter/src/run.rs @@ -10,9 +10,9 @@ use std::{ }; // non-std crates -use anyhow::{anyhow, Result}; +use anyhow::{Result, anyhow}; use clap::Parser; -use log::{set_max_level, LevelFilter}; +use log::{LevelFilter, set_max_level}; // project specific modules/crates use crate::{ @@ -20,7 +20,7 @@ use crate::{ cli::{ClangParams, Cli, CliCommand, FeedbackInput, LinesChangedOnly, RequestedVersion}, common_fs::FileFilter, logger, - rest_api::{github::GithubApiClient, RestApiClient}, + rest_api::{RestApiClient, github::GithubApiClient}, }; const VERSION: &str = env!("CARGO_PKG_VERSION"); @@ -162,7 +162,9 @@ mod test { #[tokio::test] async fn normal() { - env::remove_var("GITHUB_OUTPUT"); // avoid writing to GH_OUT in parallel-running tests + unsafe { + env::remove_var("GITHUB_OUTPUT"); // avoid writing to GH_OUT in parallel-running tests + } let result = run_main(vec![ "cpp-linter".to_string(), "-l".to_string(), @@ -177,14 +179,18 @@ mod test { #[tokio::test] async fn version_command() { - env::remove_var("GITHUB_OUTPUT"); // avoid writing to GH_OUT in parallel-running tests + unsafe { + env::remove_var("GITHUB_OUTPUT"); // avoid writing to GH_OUT in parallel-running tests + } let result = run_main(vec!["cpp-linter".to_string(), "version".to_string()]).await; assert!(result.is_ok()); } #[tokio::test] async fn force_debug_output() { - env::remove_var("GITHUB_OUTPUT"); // avoid writing to GH_OUT in parallel-running tests + unsafe { + env::remove_var("GITHUB_OUTPUT"); // avoid writing to GH_OUT in parallel-running tests + } let result = run_main(vec![ "cpp-linter".to_string(), "-l".to_string(), @@ -198,7 +204,9 @@ mod test { #[tokio::test] async fn no_version_input() { - env::remove_var("GITHUB_OUTPUT"); // avoid writing to GH_OUT in parallel-running tests + unsafe { + env::remove_var("GITHUB_OUTPUT"); // avoid writing to GH_OUT in parallel-running tests + } let result = run_main(vec![ "cpp-linter".to_string(), "-l".to_string(), @@ -211,8 +219,10 @@ mod test { #[tokio::test] async fn pre_commit_env() { - env::remove_var("GITHUB_OUTPUT"); // avoid writing to GH_OUT in parallel-running tests - env::set_var("PRE_COMMIT", "1"); + unsafe { + env::remove_var("GITHUB_OUTPUT"); // avoid writing to GH_OUT in parallel-running tests + env::set_var("PRE_COMMIT", "1"); + } let result = run_main(vec![ "cpp-linter".to_string(), "--lines-changed-only".to_string(), @@ -227,7 +237,9 @@ mod test { // This ensures no diagnostic comments are generated when analysis is explicitly skipped. #[tokio::test] async fn no_analysis() { - env::remove_var("GITHUB_OUTPUT"); // avoid writing to GH_OUT in parallel-running tests + unsafe { + env::remove_var("GITHUB_OUTPUT"); // avoid writing to GH_OUT in parallel-running tests + } let result = run_main(vec![ "cpp-linter".to_string(), "-l".to_string(), @@ -242,7 +254,9 @@ mod test { #[tokio::test] async fn bad_repo_root() { - env::remove_var("GITHUB_OUTPUT"); // avoid writing to GH_OUT in parallel-running tests + unsafe { + env::remove_var("GITHUB_OUTPUT"); // avoid writing to GH_OUT in parallel-running tests + } let result = run_main(vec![ "cpp-linter".to_string(), "--repo-root".to_string(), diff --git a/cpp-linter/tests/comments.rs b/cpp-linter/tests/comments.rs index 99cc46c7..3c30e9b4 100644 --- a/cpp-linter/tests/comments.rs +++ b/cpp-linter/tests/comments.rs @@ -62,28 +62,32 @@ impl Default for TestParams { } async fn setup(lib_root: &Path, test_params: &TestParams) { - env::set_var( - "GITHUB_EVENT_NAME", - test_params.event_t.to_string().as_str(), - ); - env::remove_var("GITHUB_OUTPUT"); // avoid writing to GH_OUT in parallel-running tests - env::set_var("GITHUB_REPOSITORY", REPO); - env::set_var("GITHUB_SHA", SHA); - env::set_var("GITHUB_TOKEN", TOKEN); - env::set_var("CI", "true"); let mut event_payload_path = NamedTempFile::new_in("./").unwrap(); - if test_params.event_t == EventType::PullRequest { - event_payload_path - .write_all(EVENT_PAYLOAD.as_bytes()) - .expect("Failed to create mock event payload."); - env::set_var("GITHUB_EVENT_PATH", event_payload_path.path()); + unsafe { + env::set_var( + "GITHUB_EVENT_NAME", + test_params.event_t.to_string().as_str(), + ); + env::remove_var("GITHUB_OUTPUT"); // avoid writing to GH_OUT in parallel-running tests + env::set_var("GITHUB_REPOSITORY", REPO); + env::set_var("GITHUB_SHA", SHA); + env::set_var("GITHUB_TOKEN", TOKEN); + env::set_var("CI", "true"); + if test_params.event_t == EventType::PullRequest { + event_payload_path + .write_all(EVENT_PAYLOAD.as_bytes()) + .expect("Failed to create mock event payload."); + env::set_var("GITHUB_EVENT_PATH", event_payload_path.path()); + } } let reset_timestamp = (Utc::now().timestamp() + 60).to_string(); let asset_path = format!("{}/{MOCK_ASSETS_PATH}", lib_root.to_str().unwrap()); let mut server = mock_server().await; - env::set_var("GITHUB_API_URL", server.url()); + unsafe { + env::set_var("GITHUB_API_URL", server.url()); + } let mut mocks = vec![]; if test_params.lines_changed_only != LinesChangedOnly::Off { diff --git a/cpp-linter/tests/paginated_changed_files.rs b/cpp-linter/tests/paginated_changed_files.rs index c2eefec0..a0e15b9a 100644 --- a/cpp-linter/tests/paginated_changed_files.rs +++ b/cpp-linter/tests/paginated_changed_files.rs @@ -8,7 +8,7 @@ use cpp_linter::{ cli::LinesChangedOnly, common_fs::FileFilter, logger, - rest_api::{github::GithubApiClient, RestApiClient}, + rest_api::{RestApiClient, github::GithubApiClient}, }; use std::{env, io::Write, path::Path}; @@ -37,29 +37,31 @@ const REMAINING_RATE_LIMIT_HEADER: &str = "x-ratelimit-remaining"; const MALFORMED_RESPONSE_PAYLOAD: &str = "{\"message\":\"Resource not accessible by integration\"}"; async fn get_paginated_changes(lib_root: &Path, test_params: &TestParams) { - env::set_var("GITHUB_REPOSITORY", REPO); - env::set_var("GITHUB_SHA", SHA); - env::set_var("GITHUB_TOKEN", TOKEN); - env::set_var("CI", "true"); - env::set_var( - "GITHUB_EVENT_NAME", - if test_params.event_t == EventType::Push { - "push" - } else { - "pull_request" - }, - ); let tmp = TempDir::new().expect("Failed to create a temp dir for test"); let mut event_payload = NamedTempFile::new_in(tmp.path()) .expect("Failed to spawn a tmp file for test event payload"); - env::set_var( - "GITHUB_EVENT_PATH", - if test_params.no_event_payload { - Path::new("no a file.txt") - } else { - event_payload.path() - }, - ); + unsafe { + env::set_var("GITHUB_REPOSITORY", REPO); + env::set_var("GITHUB_SHA", SHA); + env::set_var("GITHUB_TOKEN", TOKEN); + env::set_var("CI", "true"); + env::set_var( + "GITHUB_EVENT_NAME", + if test_params.event_t == EventType::Push { + "push" + } else { + "pull_request" + }, + ); + env::set_var( + "GITHUB_EVENT_PATH", + if test_params.no_event_payload { + Path::new("no a file.txt") + } else { + event_payload.path() + }, + ); + } if EventType::PullRequest == test_params.event_t && !test_params.fail_serde_event_payload && !test_params.no_event_payload @@ -73,7 +75,9 @@ async fn get_paginated_changes(lib_root: &Path, test_params: &TestParams) { let asset_path = format!("{}/tests/paginated_changes", lib_root.to_str().unwrap()); let mut server = mock_server().await; - env::set_var("GITHUB_API_URL", server.url()); + unsafe { + env::set_var("GITHUB_API_URL", server.url()); + } env::set_current_dir(tmp.path()).unwrap(); logger::try_init(); log::set_max_level(log::LevelFilter::Debug); @@ -151,13 +155,15 @@ async fn get_paginated_changes(lib_root: &Path, test_params: &TestParams) { Ok(files) => { assert_eq!(files.len(), 2); for file in files { - assert!(["src/demo.cpp", "src/demo.hpp"].contains( - &file - .name - .as_path() - .to_str() - .expect("Failed to get file name from path") - )); + assert!( + ["src/demo.cpp", "src/demo.hpp"].contains( + &file + .name + .as_path() + .to_str() + .expect("Failed to get file name from path") + ) + ); } } } diff --git a/cpp-linter/tests/reviews.rs b/cpp-linter/tests/reviews.rs index 6e1fa816..339961c6 100644 --- a/cpp-linter/tests/reviews.rs +++ b/cpp-linter/tests/reviews.rs @@ -72,26 +72,30 @@ fn generate_tool_summary(review_enabled: bool, force_lgtm: bool, tool_name: &str } async fn setup(lib_root: &Path, test_params: &TestParams) { - env::remove_var("GITHUB_OUTPUT"); // avoid writing to GH_OUT in parallel-running tests - env::set_var("GITHUB_EVENT_NAME", "pull_request"); - env::set_var("GITHUB_REPOSITORY", REPO); - env::set_var("GITHUB_SHA", SHA); - env::set_var("GITHUB_TOKEN", TOKEN); - env::set_var("CI", "true"); - if test_params.summary_only { - env::set_var("CPP_LINTER_PR_REVIEW_SUMMARY_ONLY", "true"); - } let mut event_payload_path = NamedTempFile::new_in("./").unwrap(); event_payload_path .write_all(EVENT_PAYLOAD.as_bytes()) .expect("Failed to create mock event payload."); - env::set_var("GITHUB_EVENT_PATH", event_payload_path.path()); + unsafe { + env::remove_var("GITHUB_OUTPUT"); // avoid writing to GH_OUT in parallel-running tests + env::set_var("GITHUB_EVENT_NAME", "pull_request"); + env::set_var("GITHUB_REPOSITORY", REPO); + env::set_var("GITHUB_SHA", SHA); + env::set_var("GITHUB_TOKEN", TOKEN); + env::set_var("CI", "true"); + if test_params.summary_only { + env::set_var("CPP_LINTER_PR_REVIEW_SUMMARY_ONLY", "true"); + } + env::set_var("GITHUB_EVENT_PATH", event_payload_path.path()); + } let clang_version = env::var("CLANG_VERSION").unwrap_or("".to_string()); let reset_timestamp = (Utc::now().timestamp() + 60).to_string(); let asset_path = format!("{}/{MOCK_ASSETS_PATH}", lib_root.to_str().unwrap()); let mut server = mock_server().await; - env::set_var("GITHUB_API_URL", server.url()); + unsafe { + env::set_var("GITHUB_API_URL", server.url()); + } let mut mocks = vec![]; let pr_endpoint = format!("/repos/{REPO}/pulls/{PR}");