refactor line filters; minor metadata updates

- moved clang-format advice filtering by lines into `run_clang_format()`
- abstracted line ranges selection into `FileObj::get_ranges()` (& added tests)
- update README link to Apache 2.0 License
- update git-ignored cache folder name
- update dev-dependency version of tempfile crate
- adjust RestApiClient function signatures for posting feedback, now that line filtering is done beforehand
- also fix a typo in the cfg attr filter (about openssl-vendored feature)

Author: 2bndy5

.gitignore

@@ -195,5 +195,5 @@ Cargo.lock
 
 
 # ignore generated files
-.cpp_linter_cache/
+.cpp-linter_cache/
 cpp-linter-py/docs/cli_args.rst

README.md

@@ -49,4 +49,4 @@ The python binding uses
 - [pyo3](https://crates.io/crates/pyo3): Dual-licensed under [Apache 2.0][Apache2] or [MIT][MIT].
 
 [MIT]: https://choosealicense.com/licenses/mit
-[Apache2]: https://github.com/clap-rs/clap/blob/HEAD/LICENSE-APACHE
+[Apache2]: https://choosealicense.com/licenses/apache-2.0/

cpp-linter-lib/Cargo.toml

@@ -23,7 +23,7 @@ serde_json = "1.0.108"
 which = "5.0.0"
 
 [dev-dependencies]
-tempfile = "3.8.1"
+tempfile = "3.9.0"
 
 [features]
 openssl-vendored = ["dep:openssl", "dep:openssl-probe"]

cpp-linter-lib/src/clang_tools/clang_format.rs

@@ -45,6 +45,18 @@ pub struct Replacement {
     pub cols: Option<usize>,
 }
 
+impl Clone for Replacement {
+    fn clone(&self) -> Self {
+        Replacement {
+            offset: self.offset,
+            length: self.length,
+            value: self.value.clone(),
+            line: self.line,
+            cols: self.cols,
+        }
+    }
+}
+
 /// Run clang-tidy for a specific `file`, then parse and return it's XML output.
 pub fn run_clang_format(
     cmd: &mut Command,
@@ -53,15 +65,9 @@ pub fn run_clang_format(
     lines_changed_only: u8,
 ) -> FormatAdvice {
     cmd.args(["--style", style, "--output-replacements-xml"]);
-    if lines_changed_only > 0 {
-        let ranges = if lines_changed_only == 2 {
-            &file.diff_chunks
-        } else {
-            &file.added_ranges
-        };
-        for range in ranges {
-            cmd.arg(format!("--lines={}:{}", range.start(), range.end()));
-        }
+    let ranges = file.get_ranges(lines_changed_only);
+    for range in &ranges {
+        cmd.arg(format!("--lines={}:{}", range.start(), range.end()));
     }
     cmd.arg(file.name.to_string_lossy().as_ref());
     log::info!(
@@ -103,11 +109,23 @@ pub fn run_clang_format(
             replacements: vec![],
         });
     if !format_advice.replacements.is_empty() {
+        let mut filtered_replacements = Vec::new();
         for replacement in &mut format_advice.replacements {
             let (line_number, columns) = get_line_cols_from_offset(&file.name, replacement.offset);
             replacement.line = Some(line_number);
             replacement.cols = Some(columns);
+            for range in &ranges {
+                if range.contains(&line_number.try_into().unwrap_or(0)) {
+                    filtered_replacements.push(replacement.clone());
+                    break;
+                }
+            }
+            if ranges.is_empty() {
+                // lines_changed_only is disabled
+                filtered_replacements.push(replacement.clone());
+            }
         }
+        format_advice.replacements = filtered_replacements;
     }
     format_advice
 }

cpp-linter-lib/src/clang_tools/clang_tidy.rs

@@ -144,30 +144,6 @@ fn parse_tidy_output(
     result
 }
 
-#[cfg(test)]
-mod test {
-    #[test]
-    fn test_capture() {
-        let src = "tests/demo/demo.hpp:11:11: warning: use a trailing return type for this function [modernize-use-trailing-return-type]";
-        let pat =
-            regex::Regex::new(r"^(.+):(\d+):(\d+):\s(\w+):(.*)\[([a-zA-Z\d\-\.]+)\]$").unwrap();
-        let cap = pat.captures(src).unwrap();
-        assert_eq!(
-            cap.get(0).unwrap().as_str(),
-            format!(
-                "{}:{}:{}: {}:{}[{}]",
-                cap.get(1).unwrap().as_str(),
-                cap.get(2).unwrap().as_str(),
-                cap.get(3).unwrap().as_str(),
-                cap.get(4).unwrap().as_str(),
-                cap.get(5).unwrap().as_str(),
-                cap.get(6).unwrap().as_str()
-            )
-            .as_str()
-        )
-    }
-}
-
 /// Run clang-tidy, then parse and return it's output.
 pub fn run_clang_tidy(
     cmd: &mut Command,
@@ -190,11 +166,7 @@ pub fn run_clang_tidy(
         }
     }
     if lines_changed_only > 0 {
-        let ranges = if lines_changed_only == 2 {
-            &file.diff_chunks
-        } else {
-            &file.added_ranges
-        };
+        let ranges = file.get_ranges(lines_changed_only);
         let filter = format!(
             "[{{\"name\":{:?},\"lines\":{:?}}}]",
             &file
@@ -230,3 +202,27 @@ pub fn run_clang_tidy(
     }
     parse_tidy_output(&output.stdout, database_json)
 }
+
+#[cfg(test)]
+mod test {
+    #[test]
+    fn test_capture() {
+        let src = "tests/demo/demo.hpp:11:11: warning: use a trailing return type for this function [modernize-use-trailing-return-type]";
+        let pat =
+            regex::Regex::new(r"^(.+):(\d+):(\d+):\s(\w+):(.*)\[([a-zA-Z\d\-\.]+)\]$").unwrap();
+        let cap = pat.captures(src).unwrap();
+        assert_eq!(
+            cap.get(0).unwrap().as_str(),
+            format!(
+                "{}:{}:{}: {}:{}[{}]",
+                cap.get(1).unwrap().as_str(),
+                cap.get(2).unwrap().as_str(),
+                cap.get(3).unwrap().as_str(),
+                cap.get(4).unwrap().as_str(),
+                cap.get(5).unwrap().as_str(),
+                cap.get(6).unwrap().as_str()
+            )
+            .as_str()
+        )
+    }
+}

cpp-linter-lib/src/common_fs.rs

@@ -68,6 +68,16 @@ impl FileObj {
         }
         ranges
     }
+
+    pub fn get_ranges(&self, lines_changed_only: u8) -> Vec<RangeInclusive<u32>> {
+        if lines_changed_only == 2 {
+            self.diff_chunks.to_vec()
+        } else if lines_changed_only == 1 {
+            self.added_ranges.to_vec()
+        } else {
+            Vec::new()
+        }
+    }
 }
 
 /// Describes if a specified `file_name` is contained within the given `set` of paths.
@@ -234,12 +244,16 @@ pub fn normalize_path(path: &Path) -> PathBuf {
 
 #[cfg(test)]
 mod test {
-
-    // *********************** tests for normalized paths
-    use super::{list_source_files, normalize_path};
     use std::env::current_dir;
+    use std::env::set_current_dir;
     use std::path::PathBuf;
 
+    use super::{get_line_cols_from_offset, list_source_files, normalize_path, FileObj};
+    use crate::cli::{get_arg_parser, parse_ignore};
+    use crate::common_fs::is_file_in_list;
+
+    // *********************** tests for normalized paths
+
     #[test]
     fn normalize_redirects() {
         let mut src = current_dir().unwrap();
@@ -274,9 +288,6 @@ mod test {
     }
 
     // ************* tests for ignored paths
-    use crate::cli::{get_arg_parser, parse_ignore};
-    use crate::common_fs::is_file_in_list;
-    use std::env::set_current_dir;
 
     fn setup_ignore(input: &str) -> (Vec<String>, Vec<String>) {
         let arg_parser = get_arg_parser();
@@ -359,6 +370,8 @@ mod test {
         ));
     }
 
+    // *********************** tests for recursive path search
+
     #[test]
     fn walk_dir_recursively() {
         let (ignored, not_ignored) = setup_ignore("target");
@@ -378,12 +391,48 @@ mod test {
         }
     }
 
-    use super::get_line_cols_from_offset;
+    // *********************** tests for translating byte offset into line/column
+
     #[test]
     fn translate_byte_offset() {
         let (lines, cols) = get_line_cols_from_offset(&PathBuf::from("tests/demo/demo.cpp"), 144);
         println!("lines: {lines}, cols: {cols}");
         assert_eq!(lines, 13);
         assert_eq!(cols, 5);
     }
+
+    // *********************** tests for FileObj::get_ranges()
+
+    #[test]
+    fn get_ranges_0() {
+        let file_obj = FileObj::new(PathBuf::from("tests/demo/demo.cpp"));
+        let ranges = file_obj.get_ranges(0);
+        assert!(ranges.is_empty());
+    }
+
+    #[test]
+    fn get_ranges_2() {
+        let diff_chunks = vec![1..=10];
+        let added_lines = vec![4, 5, 9];
+        let file_obj = FileObj::from(
+            PathBuf::from("tests/demo/demo.cpp"),
+            added_lines,
+            diff_chunks.clone(),
+        );
+        let ranges = file_obj.get_ranges(2);
+        assert_eq!(ranges, diff_chunks);
+    }
+
+    #[test]
+    fn get_ranges_1() {
+        let diff_chunks = vec![1..=10];
+        let added_lines = vec![4, 5, 9];
+        let file_obj = FileObj::from(
+            PathBuf::from("tests/demo/demo.cpp"),
+            added_lines,
+            diff_chunks,
+        );
+        let ranges = file_obj.get_ranges(1);
+        assert_eq!(ranges, vec![4..=5, 9..=9]);
+    }
 }

cpp-linter-lib/src/rest_api/github_api.rs

@@ -193,7 +193,6 @@ impl RestApiClient for GithubApiClient {
         step_summary: bool,
         file_annotations: bool,
         style: &str,
-        lines_changed_only: u8,
     ) {
         let (comment, format_checks_failed, tidy_checks_failed) =
             self.make_comment(files, format_advice, tidy_advice);
@@ -248,7 +247,7 @@ impl RestApiClient for GithubApiClient {
             }
         }
         if file_annotations {
-            self.post_annotations(files, format_advice, tidy_advice, style, lines_changed_only);
+            self.post_annotations(files, format_advice, tidy_advice, style);
         }
         if step_summary {
             self.post_step_summary(&comment);
@@ -280,7 +279,6 @@ impl GithubApiClient {
         format_advice: &[FormatAdvice],
         tidy_advice: &[Vec<TidyNotification>],
         style: &str,
-        lines_changed_only: u8,
     ) {
         if !format_advice.is_empty() {
             // formalize the style guide name
@@ -298,36 +296,18 @@ impl GithubApiClient {
                     String::from("Custom")
                 };
 
-            // iterate over clang-format and post applicable annotations (according to line filtering)
+            // iterate over clang-format advice and post annotations
             for (index, advice) in format_advice.iter().enumerate() {
-                // get the ranges of lines for the corresponding file
-                let ranges = if lines_changed_only == 0 {
-                    None
-                } else if lines_changed_only == 1 {
-                    Some(&files[index].added_ranges)
-                } else {
-                    Some(&files[index].diff_chunks)
-                };
-
-                // assemble a list of line numbers (as strings)
+                // assemble a list of line numbers
                 let mut lines: Vec<usize> = Vec::new();
                 for replacement in &advice.replacements {
                     if let Some(line_int) = replacement.line {
                         if !lines.contains(&line_int) {
-                            if let Some(line_ranges) = ranges {
-                                for line_range in line_ranges {
-                                    if line_range.contains(&line_int.try_into().unwrap()) {
-                                        lines.push(line_int);
-                                        break;
-                                    }
-                                }
-                            } else {
-                                lines.push(line_int);
-                            }
+                            lines.push(line_int);
                         }
                     }
                 }
-                // post annotation if any applicable lines were format
+                // 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})",

cpp-linter-lib/src/rest_api/mod.rs

@@ -133,6 +133,5 @@ pub trait RestApiClient {
         step_summary: bool,
         file_annotations: bool,
         style: &str,
-        lines_changed_only: u8,
     );
 }

cpp-linter-lib/src/run.rs

@@ -8,7 +8,7 @@ use std::path::{Path, PathBuf};
 
 // non-std crates
 use log::{set_max_level, LevelFilter};
-#[cfg(features = "openssl-vendored")]
+#[cfg(feature = "openssl-vendored")]
 use openssl_probe;
 
 // project specific modules/crates
@@ -19,12 +19,12 @@ use crate::github_api::GithubApiClient;
 use crate::logger::{self, end_log_group, start_log_group};
 use crate::rest_api::RestApiClient;
 
-#[cfg(features = "openssl-vendored")]
+#[cfg(feature = "openssl-vendored")]
 fn probe_ssl_certs() {
     openssl_probe::init_ssl_cert_env_vars();
 }
 
-#[cfg(not(openssl_probe))]
+#[cfg(not(feature = "openssl-vendored"))]
 fn probe_ssl_certs() {}
 
 /// This is the backend entry point for console applications.
@@ -139,7 +139,6 @@ pub fn run_main(args: Vec<String>) -> i32 {
         step_summary,
         file_annotations,
         style,
-        lines_changed_only,
     );
     end_log_group();
     0