Add --color tag so we can remove color from output, refactor output formatting

src/packs.rs

@@ -18,6 +18,8 @@ pub(crate) mod monkey_patch_detection;
 pub(crate) mod pack;
 pub(crate) mod parsing;
 pub(crate) mod raw_configuration;
+pub(crate) mod template;
+pub(crate) mod text;
 pub(crate) mod walk_directory;
 
 mod constant_dependencies;
@@ -40,6 +42,7 @@ pub(crate) use self::parsing::ruby::zeitwerk::get_zeitwerk_constant_resolver;
 pub(crate) use self::parsing::ParsedDefinition;
 pub(crate) use self::parsing::UnresolvedReference;
 use anyhow::bail;
+use cli::ColorChoice;
 use cli::OutputFormat;
 use cli::ViolationsFound;
 pub(crate) use configuration::Configuration;
@@ -49,6 +52,7 @@ pub(crate) use package_todo::PackageTodo;
 use anyhow::Context;
 use serde::Deserialize;
 use serde::Serialize;
+use std::io::IsTerminal;
 use std::path::PathBuf;
 
 pub fn greet() {
@@ -70,23 +74,44 @@ pub fn create(
     Ok(())
 }
 
+/// Determine whether to use colors based on the color choice
+fn color_mode_for(color: ColorChoice) -> text::ColorMode {
+    match color {
+        ColorChoice::Always => text::ColorMode::Colored,
+        ColorChoice::Never => text::ColorMode::Plain,
+        ColorChoice::Auto => {
+            if std::io::stdout().is_terminal() {
+                text::ColorMode::Colored
+            } else {
+                text::ColorMode::Plain
+            }
+        }
+    }
+}
+
 pub fn check(
     configuration: &Configuration,
     output_format: OutputFormat,
+    color: ColorChoice,
     files: Vec<String>,
 ) -> anyhow::Result<()> {
     let result = checker::check_all(configuration, files)
         .context("Failed to check files")?;
 
     match output_format {
         OutputFormat::Packwerk => {
-            println!("{}", result);
+            text::write_text(
+                &result,
+                configuration,
+                std::io::stdout(),
+                color_mode_for(color),
+            )?;
         }
         OutputFormat::CSV => {
-            csv::write_csv(&result, std::io::stdout())?;
+            csv::write_csv(&result, configuration, std::io::stdout())?;
         }
         OutputFormat::JSON => {
-            json::write_json(&result, std::io::stdout())?;
+            json::write_json(&result, configuration, std::io::stdout())?;
         }
     }
 

src/packs/checker.rs

@@ -4,7 +4,6 @@ pub(crate) mod layer;
 
 mod common_test;
 mod folder_privacy;
-mod output_helper;
 pub(crate) mod pack_checker;
 mod privacy;
 pub(crate) mod reference;
@@ -25,9 +24,6 @@ use rayon::prelude::IntoParallelRefIterator;
 use rayon::prelude::ParallelIterator;
 use reference::Reference;
 use std::collections::HashMap;
-use std::fmt;
-use std::fmt::Display;
-use std::fmt::Formatter;
 use std::{collections::HashSet, path::PathBuf};
 use tracing::debug;
 
@@ -36,7 +32,7 @@ use super::reference_extractor::get_all_references;
 
 #[derive(PartialEq, Clone, Eq, Hash, Debug)]
 pub struct ViolationIdentifier {
-    pub violation_type: String,
+    pub violation_type: CheckerType,
     pub strict: bool,
     pub file: String,
     pub constant_name: String,
@@ -52,11 +48,16 @@ pub struct ViolationIdentifier {
 ///   one violation, even if they occur at different lines
 /// - Keeping line/column out of the identity makes violations stable across
 ///   minor code movements that shift line numbers
+///
+/// Violations store only data - template expansion happens in formatters.
 #[derive(PartialEq, Clone, Eq, Hash, Debug)]
 pub struct Violation {
-    pub message: String,
     pub identifier: ViolationIdentifier,
     pub source_location: SourceLocation,
+    // Additional data for template expansion:
+    pub referencing_pack_relative_yml: String,
+    pub defining_layer: Option<String>,
+    pub referencing_layer: Option<String>,
 }
 
 pub(crate) trait CheckerInterface {
@@ -86,47 +87,6 @@ impl CheckAllResult {
             || !self.stale_violations.is_empty()
             || !self.strict_mode_violations.is_empty()
     }
-
-    fn write_violations(&self, f: &mut Formatter<'_>) -> fmt::Result {
-        if !self.reportable_violations.is_empty() {
-            let mut sorted_violations: Vec<&Violation> =
-                self.reportable_violations.iter().collect();
-            sorted_violations.sort_by(|a, b| a.message.cmp(&b.message));
-
-            writeln!(f, "{} violation(s) detected:", sorted_violations.len())?;
-
-            for violation in sorted_violations {
-                writeln!(f, "{}\n", violation.message)?;
-            }
-        }
-
-        if !self.stale_violations.is_empty() {
-            writeln!(
-                f,
-                "There were stale violations found, please run `{} update`",
-                bin_locater::packs_bin_name(),
-            )?;
-        }
-
-        if !self.strict_mode_violations.is_empty() {
-            for v in self.strict_mode_violations.iter() {
-                let error_message =
-                    build_strict_violation_message(&v.identifier);
-                writeln!(f, "{}", error_message)?;
-            }
-        }
-        Ok(())
-    }
-}
-
-impl Display for CheckAllResult {
-    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
-        if self.has_violations() {
-            self.write_violations(f)
-        } else {
-            write!(f, "No violations detected!")
-        }
-    }
 }
 struct CheckAllBuilder<'a> {
     configuration: &'a Configuration,
@@ -517,57 +477,5 @@ fn remove_reference_to_dependency(
     write_pack_to_disk(&updated_pack)?;
     Ok(())
 }
-#[cfg(test)]
-mod tests {
-    use std::collections::HashSet;
-
-    use crate::packs::checker::{
-        CheckAllResult, Violation, ViolationIdentifier,
-    };
-    use crate::packs::SourceLocation;
-
-    #[test]
-    fn test_write_violations() {
-        let chec_result = CheckAllResult {
-            reportable_violations: [Violation {
-                    message: "foo/bar/file1.rb:10:5\nPrivacy violation: `::Foo::PrivateClass` is private to `foo`, but referenced from `bar`".to_string(),
-                    identifier: ViolationIdentifier {
-                        violation_type: "Privacy".to_string(),
-                        strict: false,
-                        file: "foo/bar/file1.rb".to_string(),
-                        constant_name: "::Foo::PrivateClass".to_string(),
-                        referencing_pack_name: "bar".to_string(),
-                        defining_pack_name: "foo".to_string(),
-                    },
-                    source_location: SourceLocation { line: 10, column: 5 },
-                },
-                Violation {
-                    message: "foo/bar/file2.rb:15:3\nDependency violation: `::Foo::AnotherClass` is not allowed to depend on `::Bar::SomeClass`".to_string(),
-                    identifier: ViolationIdentifier {
-                        violation_type: "Dependency".to_string(),
-                        strict: false,
-                        file: "foo/bar/file2.rb".to_string(),
-                        constant_name: "::Foo::AnotherClass".to_string(),
-                        referencing_pack_name: "foo".to_string(),
-                        defining_pack_name: "bar".to_string(),
-                     },
-                     source_location: SourceLocation { line: 15, column: 3 },
-                }].iter().cloned().collect(),
-            stale_violations: Vec::new(),
-            strict_mode_violations: HashSet::new(),
-        };
-
-        let expected_output = "2 violation(s) detected:
-foo/bar/file1.rb:10:5
-Privacy violation: `::Foo::PrivateClass` is private to `foo`, but referenced from `bar`
-
-foo/bar/file2.rb:15:3
-Dependency violation: `::Foo::AnotherClass` is not allowed to depend on `::Bar::SomeClass`
-
-";
-
-        let actual = format!("{}", chec_result);
-
-        assert_eq!(actual, expected_output);
-    }
-}
+// Note: Display impl was removed from CheckAllResult. Use write_text() directly with Configuration.
+// Tests for text formatting are in text.rs

src/packs/checker/common_test.rs

@@ -7,6 +7,7 @@ pub mod tests {
         checker::{
             reference::Reference, CheckerInterface, ViolationIdentifier,
         },
+        checker_configuration::CheckerType,
         pack::Pack,
         Configuration, PackSet, SourceLocation, Violation,
     };
@@ -26,26 +27,46 @@ pub mod tests {
     }
 
     pub fn build_expected_violation(
-        message: String,
-        violation_type: String,
+        violation_type: CheckerType,
         strict: bool,
     ) -> Violation {
         build_expected_violation_with_constant(
-            message,
             violation_type,
             strict,
             String::from("::Bar"),
         )
     }
 
+    pub fn build_expected_violation_with_layers(
+        violation_type: CheckerType,
+        strict: bool,
+        defining_layer: &str,
+        referencing_layer: &str,
+    ) -> Violation {
+        Violation {
+            identifier: ViolationIdentifier {
+                violation_type,
+                strict,
+                file: String::from("packs/foo/app/services/foo.rb"),
+                constant_name: String::from("::Bar"),
+                referencing_pack_name: String::from("packs/foo"),
+                defining_pack_name: String::from("packs/bar"),
+            },
+            source_location: SourceLocation { line: 3, column: 1 },
+            referencing_pack_relative_yml: String::from(
+                "packs/foo/package.yml",
+            ),
+            defining_layer: Some(defining_layer.to_string()),
+            referencing_layer: Some(referencing_layer.to_string()),
+        }
+    }
+
     pub fn build_expected_violation_with_constant(
-        message: String,
-        violation_type: String,
+        violation_type: CheckerType,
         strict: bool,
         constant_name: String,
     ) -> Violation {
         Violation {
-            message,
             identifier: ViolationIdentifier {
                 violation_type,
                 strict,
@@ -55,6 +76,11 @@ pub mod tests {
                 defining_pack_name: String::from("packs/bar"),
             },
             source_location: SourceLocation { line: 3, column: 1 },
+            referencing_pack_relative_yml: String::from(
+                "packs/foo/package.yml",
+            ),
+            defining_layer: None,
+            referencing_layer: None,
         }
     }
 
@@ -79,8 +105,10 @@ pub mod tests {
     }
 
     pub fn default_referencing_pack() -> Pack {
+        use std::path::PathBuf;
         Pack {
             name: "packs/foo".to_owned(),
+            relative_path: PathBuf::from("packs/foo"),
             ..Pack::default()
         }
     }
@@ -138,21 +166,7 @@ pub mod tests {
 
         let result = checker.check(&reference, &configuration)?;
 
-        let stripped_result = match result {
-            Some(violation) => {
-                let stripped_message =
-                    strip_ansi_escapes::strip(violation.message);
-
-                Some(Violation {
-                    message: String::from_utf8_lossy(&stripped_message)
-                        .to_string(),
-                    ..violation
-                })
-            }
-            None => None,
-        };
-
-        assert_eq!(stripped_result, test_checker.expected_violation);
+        assert_eq!(result, test_checker.expected_violation);
 
         Ok(())
     }

src/packs/checker/dependency.rs

@@ -220,13 +220,15 @@ mod tests {
                 name: "packs/bar".to_owned(),
                 ..default_defining_pack()
             }),
-            referencing_pack: Pack{
+            referencing_pack: Pack {
                 relative_path: PathBuf::from("packs/foo"),
                 enforce_dependencies: Some(CheckerSetting::True),
-                ..default_referencing_pack()},
+                ..default_referencing_pack()
+            },
             expected_violation: Some(build_expected_violation(
-                "packs/foo/app/services/foo.rb:3:1\nDependency violation: `::Bar` belongs to `packs/bar`, but `packs/foo/package.yml` does not specify a dependency on `packs/bar`.".to_string(),
-                "dependency".to_string(), false)),
+                CheckerType::Dependency,
+                false,
+            )),
         };
         test_check(
             &Checker {
@@ -248,13 +250,15 @@ mod tests {
                 name: "packs/bar".to_owned(),
                 ..default_defining_pack()
             }),
-            referencing_pack: Pack{
+            referencing_pack: Pack {
                 relative_path: PathBuf::from("packs/foo"),
                 enforce_dependencies: Some(CheckerSetting::Strict),
-                ..default_referencing_pack()},
+                ..default_referencing_pack()
+            },
             expected_violation: Some(build_expected_violation(
-                "packs/foo/app/services/foo.rb:3:1\nDependency violation: `::Bar` belongs to `packs/bar`, but `packs/foo/package.yml` does not specify a dependency on `packs/bar`.".to_string(),
-                "dependency".to_string(), true)),
+                CheckerType::Dependency,
+                true,
+            )),
         };
         test_check(
             &Checker {