Support a human readable output format (#41)

Adds `--output-format=sarif|text` (default `sarif`) to allow us to
produce more readable outputs. For now it's just `path:line:col msg
(severity)`

src/main.rs

@@ -4,55 +4,33 @@ use horton::config::Conf;
 use horton::diagnostic;
 use horton::rules::if_change_then_change::ictc;
 use horton::rules::pls_no_land::pls_no_land;
-use horton::run::{Cli, Run, Subcommands};
+use horton::run::{Cli, OutputFormat, Run, Subcommands};
 
 use serde_sarif::sarif;
 use std::path::PathBuf;
 use std::time::Instant;
 
-fn run() -> anyhow::Result<()> {
-    let start = Instant::now();
-    let cli: Cli = Cli::parse();
-
-    if let Some(Subcommands::Genconfig {}) = &cli.subcommand {
-        Conf::print_default();
-        return Ok(());
-    }
-
-    let mut ret = diagnostic::Diagnostics::default();
-
-    let config = Conf::builder()
-        .env()
-        .file("toolbox.toml")
-        .file(".config/toolbox.toml")
-        .file(".trunk/config/toolbox.toml")
-        .file(".trunk/configs/toolbox.toml")
-        .load()
-        .unwrap_or_else(|err| {
-            eprintln!("Toolbox cannot run: {}", err);
-            std::process::exit(1);
-        });
-
-    let run = Run {
-        paths: cli.files.into_iter().map(PathBuf::from).collect(),
-        config,
-    };
-
-    let (pls_no_land_result, ictc_result): (Result<_, _>, Result<_, _>) =
-        rayon::join(|| pls_no_land(&run), || ictc(&run, &cli.upstream));
-
-    match pls_no_land_result {
-        Ok(result) => ret.diagnostics.extend(result),
-        Err(e) => return Err(e),
-    }
-
-    match ictc_result {
-        Ok(result) => ret.diagnostics.extend(result),
-        Err(e) => return Err(e),
-    }
+fn generate_line_string(original_results: &diagnostic::Diagnostics) -> String {
+    return original_results
+        .diagnostics
+        .iter()
+        .map(|d| {
+            format!(
+                "{}:{}:{}: {} ({})",
+                d.range.path, d.range.start.line, d.range.start.character, d.message, d.severity
+            )
+        })
+        .collect::<Vec<String>>()
+        .join("\n");
+}
 
+fn generate_sarif_string(
+    original_results: &diagnostic::Diagnostics,
+    run_context: &Run,
+    start_time: &Instant,
+) -> anyhow::Result<String> {
     // TODO(sam): figure out how to stop using unwrap() inside the map() calls below
-    let mut results: Vec<sarif::Result> = ret
+    let mut results: Vec<sarif::Result> = original_results
         .diagnostics
         .iter()
         .map(|d| {
@@ -98,8 +76,8 @@ fn run() -> anyhow::Result<()> {
             sarif::MessageBuilder::default()
                 .text(format!(
                     "{:?} files processed in {:?}",
-                    run.paths.len(),
-                    start.elapsed()
+                    run_context.paths.len(),
+                    start_time.elapsed()
                 ))
                 .build()
                 .unwrap(),
@@ -131,11 +109,59 @@ fn run() -> anyhow::Result<()> {
         .build()?;
 
     let sarif = serde_json::to_string_pretty(&sarif_built)?;
+    Ok(sarif)
+}
+
+fn run() -> anyhow::Result<()> {
+    let start = Instant::now();
+    let cli: Cli = Cli::parse();
+
+    if let Some(Subcommands::Genconfig {}) = &cli.subcommand {
+        Conf::print_default();
+        return Ok(());
+    }
+
+    let mut ret = diagnostic::Diagnostics::default();
+
+    let config = Conf::builder()
+        .env()
+        .file("toolbox.toml")
+        .file(".config/toolbox.toml")
+        .file(".trunk/config/toolbox.toml")
+        .file(".trunk/configs/toolbox.toml")
+        .load()
+        .unwrap_or_else(|err| {
+            eprintln!("Toolbox cannot run: {}", err);
+            std::process::exit(1);
+        });
+
+    let run = Run {
+        paths: cli.files.into_iter().map(PathBuf::from).collect(),
+        config,
+    };
+
+    let (pls_no_land_result, ictc_result): (Result<_, _>, Result<_, _>) =
+        rayon::join(|| pls_no_land(&run), || ictc(&run, &cli.upstream));
+
+    match pls_no_land_result {
+        Ok(result) => ret.diagnostics.extend(result),
+        Err(e) => return Err(e),
+    }
+
+    match ictc_result {
+        Ok(result) => ret.diagnostics.extend(result),
+        Err(e) => return Err(e),
+    }
+
+    let mut output_string = generate_line_string(&ret);
+    if cli.output_format == OutputFormat::Sarif {
+        output_string = generate_sarif_string(&ret, &run, &start)?;
+    }
 
     if let Some(outfile) = &cli.results {
-        std::fs::write(outfile, sarif)?;
+        std::fs::write(outfile, output_string)?;
     } else {
-        println!("{}", sarif);
+        println!("{}", output_string);
     }
 
     Ok(())

src/run.rs

@@ -2,7 +2,27 @@ use crate::config::Conf;
 use std::collections::HashSet;
 use std::path::PathBuf;
 
-use clap::{Parser, Subcommand};
+use clap::builder::PossibleValue;
+use clap::{Parser, Subcommand, ValueEnum};
+
+#[derive(Debug, Copy, Clone, Eq, PartialEq)]
+pub enum OutputFormat {
+    Sarif,
+    Text,
+}
+
+impl ValueEnum for OutputFormat {
+    fn value_variants<'a>() -> &'a [Self] {
+        &[Self::Sarif, Self::Text]
+    }
+
+    fn to_possible_value(&self) -> Option<PossibleValue> {
+        Some(match self {
+            Self::Sarif => PossibleValue::new("sarif"),
+            Self::Text => PossibleValue::new("text"),
+        })
+    }
+}
 
 #[derive(Parser, Debug)]
 #[clap(version = env!("CARGO_PKG_VERSION"), author = "Trunk Technologies Inc.")]
@@ -16,6 +36,9 @@ pub struct Cli {
     #[arg(default_value_t = String::from("HEAD"))]
     pub upstream: String,
 
+    #[clap(long, default_value = "sarif")]
+    pub output_format: OutputFormat,
+
     #[clap(long)]
     /// optional path to write results to
     pub results: Option<String>,

tests/integration_testing.rs

@@ -115,10 +115,14 @@ impl TestRepo {
     }
 
     pub fn run_horton(&self) -> anyhow::Result<HortonOutput> {
-        self.run_horton_against("HEAD")
+        self.run_horton_with("HEAD", "sarif")
     }
 
-    pub fn run_horton_against(&self, upstream_ref: &str) -> anyhow::Result<HortonOutput> {
+    pub fn run_horton_with(
+        &self,
+        upstream_ref: &str,
+        format: &str,
+    ) -> anyhow::Result<HortonOutput> {
         let mut cmd = Command::cargo_bin("trunk-toolbox")?;
 
         let modified_paths =
@@ -132,6 +136,7 @@ impl TestRepo {
         cmd.arg("--upstream")
             .arg(upstream_ref)
             .current_dir(self.dir.path());
+        cmd.arg("--output-format").arg(format);
         for path in strings.unwrap() {
             cmd.arg(path);
         }

tests/main_test.rs

@@ -25,7 +25,7 @@ fn binary_file_committed() -> anyhow::Result<()> {
     test_repo.write("picture.binary", include_bytes!("trunk-logo.png"));
     test_repo.git_commit_all("commit a picture");
 
-    let horton = test_repo.run_horton_against("HEAD^")?;
+    let horton = test_repo.run_horton_with("HEAD^", "sarif")?;
 
     assert_that(&horton.exit_code).contains_value(0);
     assert_that(&horton.stdout.contains("Expected change")).is_false();
@@ -67,7 +67,7 @@ fn lfs_file_committed() -> anyhow::Result<()> {
 
     test_repo.write("picture.binary", include_bytes!("trunk-logo.png"));
 
-    let horton = test_repo.run_horton_against("HEAD^")?;
+    let horton = test_repo.run_horton_with("HEAD^", "sarif")?;
 
     assert_that(&horton.exit_code).contains_value(0);
     assert_that(&horton.stdout.contains("Expected change")).is_false();

tests/output.sarif

@@ -0,0 +1,65 @@
+{
+  "runs": [
+    {
+      "results": [
+        {
+          "level": "error",
+          "locations": [
+            {
+              "physicalLocation": {
+                "artifactLocation": {
+                  "uri": "alpha.foo"
+                },
+                "region": {
+                  "endColumn": 12,
+                  "endLine": 2,
+                  "startColumn": 1,
+                  "startLine": 2
+                }
+              }
+            }
+          ],
+          "message": {
+            "text": "Found 'do-NOT-lAnD'"
+          },
+          "ruleId": "do-not-land"
+        },
+        {
+          "level": "error",
+          "locations": [
+            {
+              "physicalLocation": {
+                "artifactLocation": {
+                  "uri": "alpha.foo"
+                },
+                "region": {
+                  "endColumn": 10,
+                  "endLine": 3,
+                  "startColumn": 1,
+                  "startLine": 3
+                }
+              }
+            }
+          ],
+          "message": {
+            "text": "Found 'DONOTLAND'"
+          },
+          "ruleId": "do-not-land"
+        },
+        {
+          "level": "note",
+          "message": {
+            "text": "1 files processed in ms"
+          },
+          "ruleId": "toolbox-perf"
+        }
+      ],
+      "tool": {
+        "driver": {
+          "name": "trunk-toolbox"
+        }
+      }
+    }
+  ],
+  "version": "2.1.0"
+}

tests/output_format_test.rs

@@ -0,0 +1,53 @@
+// trunk-ignore-all(trunk-toolbox/do-not-land)
+extern crate regex;
+
+use spectral::prelude::*;
+use std::fs;
+use std::path::Path;
+
+use regex::Regex;
+mod integration_testing;
+use integration_testing::TestRepo;
+
+#[test]
+fn default_sarif() -> anyhow::Result<()> {
+    let test_repo = TestRepo::make()?;
+
+    let expected_file = Path::new(file!()).parent().unwrap().join("output.sarif");
+    let expected_sarif = fs::read_to_string(expected_file)?;
+
+    test_repo.write(
+        "alpha.foo",
+        "lorem ipsum dolor\ndo-NOT-lAnD\nDONOTLAND sit amet\n".as_bytes(),
+    );
+    test_repo.git_add_all()?;
+    let horton = test_repo.run_horton()?;
+
+    let re = Regex::new(r"\d+\.\d+ms").unwrap();
+    let normalized_output = re.replace(&horton.stdout, "ms");
+
+    assert_that(&horton.exit_code).contains_value(0);
+    assert_that(&normalized_output.to_string()).is_equal_to(expected_sarif);
+
+    Ok(())
+}
+
+#[test]
+fn default_print() -> anyhow::Result<()> {
+    let test_repo = TestRepo::make()?;
+
+    test_repo.write(
+        "alpha.foo",
+        "lorem ipsum dolor\ndo-NOT-lAnD\nDONOTLAND sit amet\n".as_bytes(),
+    );
+    test_repo.git_add_all()?;
+    let horton = test_repo.run_horton_with("HEAD", "text")?;
+    let expected_text = String::from(
+        "alpha.foo:1:0: Found 'do-NOT-lAnD' (error)\nalpha.foo:2:0: Found 'DONOTLAND' (error)\n",
+    );
+
+    assert_that(&horton.exit_code).contains_value(0);
+    assert_that(&horton.stdout).is_equal_to(&expected_text);
+
+    Ok(())
+}

tests/todo_test.rs

@@ -24,7 +24,7 @@ fn basic_todo() -> anyhow::Result<()> {
         "alpha.foo",
         "lorem ipsum dolor\ntoDO\nsit amet\n".as_bytes(),
     );
-    write_enable_config(&test_repo);
+    write_enable_config(&test_repo)?;
     test_repo.git_add_all()?;
     let horton = test_repo.run_horton()?;
 
@@ -42,7 +42,7 @@ fn basic_fixme() -> anyhow::Result<()> {
         "alpha.foo",
         "lorem ipsum dolor\nFIXME: fix this\nsit amet\n".as_bytes(),
     );
-    write_enable_config(&test_repo);
+    write_enable_config(&test_repo)?;
     test_repo.git_add_all()?;
     let horton = test_repo.run_horton()?;
 
@@ -60,7 +60,7 @@ fn basic_mastodon() -> anyhow::Result<()> {
         "alpha.foo",
         "lorem ipsum dolor\n// Mastodons are cool\nsit amet\n".as_bytes(),
     );
-    write_enable_config(&test_repo);
+    write_enable_config(&test_repo)?;
     test_repo.git_add_all()?;
     let horton = test_repo.run_horton()?;