Some updates from py codebase (#5)

* encapsulate user inputs to post_feedback()
* encapsulate `TidyNotification`s in a `TidyAdvice` struct
* change data type returned on REST API calls
* re-order feedback based on fallibility
* use an enum to statically type `--lines-changed-only`

Author: 2bndy5

.pre-commit-config.yaml

@@ -1,6 +1,6 @@
 repos:
   - repo: https://github.com/pre-commit/pre-commit-hooks
-    rev: v4.5.0
+    rev: v4.6.0
     hooks:
       - id: trailing-whitespace
         exclude: cpp-linter-lib/tests/capture_tools_output/cpp-linter/cpp-linter/test_git_lib.patch
@@ -14,7 +14,7 @@ repos:
       - id: mixed-line-ending
         args: ["--fix=lf"]
   - repo: https://github.com/astral-sh/ruff-pre-commit
-    rev: v0.1.8
+    rev: v0.5.2
     hooks:
       # Run the python linter.
       - id: ruff

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

@@ -8,7 +8,10 @@ use serde::Deserialize;
 use serde_xml_rs::de::Deserializer;
 
 // project-specific crates/modules
-use crate::common_fs::{get_line_cols_from_offset, FileObj};
+use crate::{
+    cli::LinesChangedOnly,
+    common_fs::{get_line_cols_from_offset, FileObj},
+};
 
 /// A Structure used to deserialize clang-format's XML output.
 #[derive(Debug, Deserialize, PartialEq)]
@@ -62,7 +65,7 @@ pub fn run_clang_format(
     cmd: &mut Command,
     file: &FileObj,
     style: &str,
-    lines_changed_only: u8,
+    lines_changed_only: &LinesChangedOnly,
 ) -> FormatAdvice {
     cmd.args(["--style", style, "--output-replacements-xml"]);
     let ranges = file.get_ranges(lines_changed_only);

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

@@ -12,7 +12,10 @@ use regex::Regex;
 use serde::Deserialize;
 
 // project-specific modules/crates
-use crate::common_fs::{normalize_path, FileObj};
+use crate::{
+    cli::LinesChangedOnly,
+    common_fs::{normalize_path, FileObj},
+};
 
 /// Used to deserialize a JSON compilation database
 #[derive(Deserialize, Debug)]
@@ -68,14 +71,34 @@ pub struct TidyNotification {
     pub suggestion: Vec<String>,
 }
 
+impl TidyNotification {
+    pub fn diagnostic_link(&self) -> String {
+        let ret_val = if let Some((category, name)) = self.diagnostic.split_once('-') {
+            format!(
+                "[{}](https://clang.llvm.org/extra/clang-tidy/checks/{category}/{name}).html",
+                self.diagnostic
+            )
+        } else {
+            self.diagnostic.clone()
+        };
+        ret_val
+    }
+}
+
+/// A struct to hold notification from clang-tidy about a single file
+pub struct TidyAdvice {
+    /// A list of notifications parsed from clang-tidy stdout.
+    pub notes: Vec<TidyNotification>,
+}
+
 /// Parses clang-tidy stdout.
 ///
 /// Here it helps to have the JSON database deserialized for normalizing paths present
 /// in the notifications.
 fn parse_tidy_output(
     tidy_stdout: &[u8],
     database_json: &Option<CompilationDatabase>,
-) -> Vec<TidyNotification> {
+) -> TidyAdvice {
     let note_header = Regex::new(r"^(.+):(\d+):(\d+):\s(\w+):(.*)\[([a-zA-Z\d\-\.]+)\]$").unwrap();
     let mut notification = None;
     let mut result = Vec::new();
@@ -141,19 +164,19 @@ fn parse_tidy_output(
     if let Some(note) = notification {
         result.push(note);
     }
-    result
+    TidyAdvice { notes: result }
 }
 
 /// Run clang-tidy, then parse and return it's output.
 pub fn run_clang_tidy(
     cmd: &mut Command,
     file: &FileObj,
     checks: &str,
-    lines_changed_only: u8,
+    lines_changed_only: &LinesChangedOnly,
     database: &Option<PathBuf>,
     extra_args: &Option<Vec<&str>>,
     database_json: &Option<CompilationDatabase>,
-) -> Vec<TidyNotification> {
+) -> TidyAdvice {
     if !checks.is_empty() {
         cmd.args(["-checks", checks]);
     }
@@ -165,7 +188,7 @@ pub fn run_clang_tidy(
             cmd.args(["--extra-arg", format!("\"{}\"", arg).as_str()]);
         }
     }
-    if lines_changed_only > 0 {
+    if *lines_changed_only != LinesChangedOnly::Off {
         let ranges = file.get_ranges(lines_changed_only);
         let filter = format!(
             "[{{\"name\":{:?},\"lines\":{:?}}}]",

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

@@ -10,11 +10,14 @@ use which::{which, which_in};
 
 // project-specific modules/crates
 use super::common_fs::FileObj;
-use crate::logger::{end_log_group, start_log_group};
+use crate::{
+    cli::LinesChangedOnly,
+    logger::{end_log_group, start_log_group},
+};
 pub mod clang_format;
 use clang_format::{run_clang_format, FormatAdvice};
 pub mod clang_tidy;
-use clang_tidy::{run_clang_tidy, CompilationDatabase, TidyNotification};
+use clang_tidy::{run_clang_tidy, CompilationDatabase, TidyAdvice};
 
 /// Fetch the path to a clang tool by `name` (ie `"clang-tidy"` or `"clang-format"`) and
 /// `version`.
@@ -69,7 +72,7 @@ pub fn get_clang_tool_exe(name: &str, version: &str) -> Result<PathBuf, &'static
 /// Runs clang-tidy and/or clang-format and returns the parsed output from each.
 ///
 /// The returned list of [`FormatAdvice`] is parallel to the `files` list passed in
-/// here. The returned 2D list of [`TidyNotification`] is also parallel on the first
+/// here. The returned 2D list of [`TidyAdvice`] is also parallel on the first
 /// dimension. The second dimension is a list of notes specific to a translation unit
 /// (each element of `files`).
 ///
@@ -80,10 +83,10 @@ pub fn capture_clang_tools_output(
     version: &str,
     tidy_checks: &str,
     style: &str,
-    lines_changed_only: u8,
+    lines_changed_only: &LinesChangedOnly,
     database: Option<PathBuf>,
     extra_args: Option<Vec<&str>>,
-) -> (Vec<FormatAdvice>, Vec<Vec<TidyNotification>>) {
+) -> (Vec<FormatAdvice>, Vec<TidyAdvice>) {
     // find the executable paths for clang-tidy and/or clang-format and show version
     // info as debugging output.
     let clang_tidy_command = if tidy_checks != "-*" {
@@ -126,9 +129,8 @@ pub fn capture_clang_tools_output(
     };
 
     // iterate over the discovered files and run the clang tools
-    let mut all_format_advice: Vec<clang_format::FormatAdvice> = Vec::with_capacity(files.len());
-    let mut all_tidy_advice: Vec<Vec<clang_tidy::TidyNotification>> =
-        Vec::with_capacity(files.len());
+    let mut all_format_advice: Vec<FormatAdvice> = Vec::with_capacity(files.len());
+    let mut all_tidy_advice: Vec<TidyAdvice> = Vec::with_capacity(files.len());
     for file in files {
         start_log_group(format!("Analyzing {}", file.name.to_string_lossy()));
         if let Some(tidy_cmd) = &clang_tidy_command {

cpp-linter-lib/src/cli.rs

@@ -6,6 +6,17 @@ use std::fs;
 use clap::builder::FalseyValueParser;
 use clap::{Arg, ArgAction, ArgMatches, Command};
 
+/// An enum to describe `--lines-changed-only` CLI option's behavior.
+#[derive(PartialEq)]
+pub enum LinesChangedOnly {
+    /// All lines are scanned
+    Off,
+    /// Only lines in the diff are scanned
+    Diff,
+    /// Only lines in the diff with additions are scanned.
+    On,
+}
+
 /// Builds and returns the Command Line Interface's argument parsing object.
 pub fn get_arg_parser() -> Command {
     Command::new("cpp-linter")

cpp-linter-lib/src/common_fs.rs

@@ -5,6 +5,8 @@ use std::path::{Component, Path};
 use std::{fs, io};
 use std::{ops::RangeInclusive, path::PathBuf};
 
+use crate::cli::LinesChangedOnly;
+
 /// A structure to represent a file's path and line changes.
 #[derive(Debug)]
 pub struct FileObj {
@@ -52,7 +54,7 @@ impl FileObj {
     /// A helper function to consolidate a [Vec<u32>] of line numbers into a
     /// [Vec<RangeInclusive<u32>>] in which each range describes the beginning and
     /// ending of a group of consecutive line numbers.
-    fn consolidate_numbers_to_ranges(lines: &Vec<u32>) -> Vec<RangeInclusive<u32>> {
+    fn consolidate_numbers_to_ranges(lines: &[u32]) -> Vec<RangeInclusive<u32>> {
         let mut range_start = None;
         let mut ranges: Vec<RangeInclusive<u32>> = Vec::new();
         for (index, number) in lines.iter().enumerate() {
@@ -69,13 +71,11 @@ impl FileObj {
         ranges
     }
 
-    pub fn get_ranges(&self, lines_changed_only: u8) -> Vec<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()
+    pub fn get_ranges(&self, lines_changed_only: &LinesChangedOnly) -> Vec<RangeInclusive<u32>> {
+        match lines_changed_only {
+            LinesChangedOnly::Diff => self.diff_chunks.to_vec(),
+            LinesChangedOnly::On => self.added_ranges.to_vec(),
+            _ => Vec::new(),
         }
     }
 }
@@ -249,6 +249,7 @@ mod test {
     use std::path::PathBuf;
 
     use super::{get_line_cols_from_offset, list_source_files, normalize_path, FileObj};
+    use crate::cli::LinesChangedOnly;
     use crate::cli::{get_arg_parser, parse_ignore};
     use crate::common_fs::is_file_in_list;
 
@@ -406,7 +407,7 @@ mod test {
     #[test]
     fn get_ranges_0() {
         let file_obj = FileObj::new(PathBuf::from("tests/demo/demo.cpp"));
-        let ranges = file_obj.get_ranges(0);
+        let ranges = file_obj.get_ranges(&LinesChangedOnly::Off);
         assert!(ranges.is_empty());
     }
 
@@ -419,7 +420,7 @@ mod test {
             added_lines,
             diff_chunks.clone(),
         );
-        let ranges = file_obj.get_ranges(2);
+        let ranges = file_obj.get_ranges(&LinesChangedOnly::Diff);
         assert_eq!(ranges, diff_chunks);
     }
 
@@ -432,7 +433,7 @@ mod test {
             added_lines,
             diff_chunks,
         );
-        let ranges = file_obj.get_ranges(1);
+        let ranges = file_obj.get_ranges(&LinesChangedOnly::On);
         assert_eq!(ranges, vec![4..=5, 9..=9]);
     }
 }