Centralize template expansion in dedicated module

Template expansion was scattered between checkers and formatters, making
it difficult to maintain consistency between default and user-supplied
templates. The reference_location placeholder needed special handling
per output format.

Changes:
- Add template.rs with expand(), build_violation_vars(), and helpers
- Move all template expansion from pack_checker to formatters
- Violation struct now stores data only (no pre-expanded message)
- ViolationIdentifier.violation_type is now CheckerType enum
- Each formatter handles reference_location appropriately:
  - text: colorized or plain based on color mode
  - json: cleared (location in separate fields)
  - csv: plain format
- Default templates restored to include {{reference_location}}

Author: martinemde

src/packs.rs

@@ -15,6 +15,7 @@ pub(crate) mod dependencies;
 pub(crate) mod ignored;
 pub(crate) mod json;
 pub(crate) mod monkey_patch_detection;
+pub(crate) mod template;
 pub(crate) mod text;
 pub(crate) mod pack;
 pub(crate) mod parsing;
@@ -99,13 +100,18 @@ pub fn check(
 
     match output_format {
         OutputFormat::Packwerk => {
-            text::write_text(&result, std::io::stdout(), color_mode_for(color))?;
+            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

@@ -24,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;
 
@@ -35,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,13 +49,15 @@ pub struct ViolationIdentifier {
 /// - Keeping line/column out of the identity makes violations stable across
 ///   minor code movements that shift line numbers
 ///
-/// `message` contains the violation description without the file location prefix.
-/// Formatters are responsible for combining source_location with message as needed.
+/// 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 {
@@ -82,76 +81,12 @@ pub struct CheckAllResult {
     pub strict_mode_violations: HashSet<Violation>,
 }
 
-const REFERENCE_LOCATION_PLACEHOLDER: &str = "{{reference_location}}";
-
 impl CheckAllResult {
     pub fn has_violations(&self) -> bool {
         !self.reportable_violations.is_empty()
             || !self.stale_violations.is_empty()
             || !self.strict_mode_violations.is_empty()
     }
-
-    /// Format a violation message, substituting `{{reference_location}}` if present.
-    fn format_violation_message(violation: &Violation) -> String {
-        let location = format!(
-            "{}:{}:{}",
-            violation.identifier.file,
-            violation.source_location.line,
-            violation.source_location.column
-        );
-
-        if violation.message.contains(REFERENCE_LOCATION_PLACEHOLDER) {
-            // Custom template uses {{reference_location}} - substitute it
-            violation
-                .message
-                .replace(REFERENCE_LOCATION_PLACEHOLDER, &format!("{}\n", location))
-        } else {
-            // Default template - prepend location
-            format!("{}\n{}", location, violation.message)
-        }
-    }
-
-    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 {
-                let formatted = Self::format_violation_message(violation);
-                writeln!(f, "{}\n", formatted)?;
-            }
-        }
-
-        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,
@@ -542,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 check_result = CheckAllResult {
-            reportable_violations: [Violation {
-                    message: "Privacy 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: "Dependency 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/file2.rb:15:3
-Dependency violation: `::Foo::AnotherClass` is not allowed to depend on `::Bar::SomeClass`
-
-foo/bar/file1.rb:10:5
-Privacy violation: `::Foo::PrivateClass` is private to `foo`, but referenced from `bar`
-
-";
-
-        let actual = format!("{}", check_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,40 @@ 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"),
-        )
+        build_expected_violation_with_constant(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 +70,9 @@ 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 +97,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()
         }
     }

src/packs/checker/dependency.rs

@@ -225,8 +225,7 @@ mod tests {
                 enforce_dependencies: Some(CheckerSetting::True),
                 ..default_referencing_pack()},
             expected_violation: Some(build_expected_violation(
-                "Dependency 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 {
@@ -253,8 +252,7 @@ mod tests {
                 enforce_dependencies: Some(CheckerSetting::Strict),
                 ..default_referencing_pack()},
             expected_violation: Some(build_expected_violation(
-                "Dependency 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 {

src/packs/checker/folder_privacy.rs

@@ -89,8 +89,7 @@ mod tests {
                 relative_path: PathBuf::from("packs/foo"),
                 ..default_referencing_pack()},
             expected_violation: Some(build_expected_violation(
-                "Folder Privacy violation: `::Bar` belongs to `packs/bar`, which is private to `packs/foo` as it is not a sibling pack or parent pack.".to_string(),
-                "folder_privacy".to_string(), false)),
+                CheckerType::FolderPrivacy, false)),
         };
         test_check(
             &Checker {
@@ -154,8 +153,7 @@ mod tests {
                 relative_path: PathBuf::from("packs/foo"),
                 ..default_referencing_pack()},
             expected_violation: Some(build_expected_violation(
-                "Folder Privacy violation: `::Bar` belongs to `packs/bar`, which is private to `packs/foo` as it is not a sibling pack or parent pack.".to_string(),
-                "folder_privacy".to_string(), true)),
+                CheckerType::FolderPrivacy, true)),
         };
         test_check(
             &Checker {

src/packs/checker/layer.rs

@@ -1,5 +1,3 @@
-use std::collections::HashMap;
-
 use super::pack_checker::PackChecker;
 use super::{CheckerInterface, ValidatorInterface};
 use crate::packs::checker::Reference;
@@ -120,10 +118,7 @@ impl CheckerInterface for Checker {
                     return Ok(None);
                 }
 
-                let mut map: HashMap<&str, String> = HashMap::new();
-                map.insert("{{defining_layer}}", defining_layer.into());
-                map.insert("{{referencing_layer}}", referencing_layer.into());
-                pack_checker.violation(Some(map))
+                pack_checker.violation(Some((defining_layer, referencing_layer)))
             }
             _ => Ok(None),
         }
@@ -141,7 +136,7 @@ mod tests {
     use std::path::PathBuf;
 
     use crate::packs::checker::common_test::tests::{
-        build_expected_violation, default_defining_pack,
+        build_expected_violation_with_layers, default_defining_pack,
         default_referencing_pack, test_check, TestChecker,
     };
     use crate::packs::checker_configuration::CheckerType;
@@ -204,9 +199,8 @@ mod tests {
                 layer: Some("utilities".to_string()),
                 ..default_referencing_pack()
             },
-            expected_violation: Some(build_expected_violation(
-                "Layer violation: `::Bar` belongs to `packs/bar` (whose layer is `product`) cannot be accessed from `packs/foo` (whose layer is `utilities`)".to_string(),
-                "layer".to_string(), false)),
+            expected_violation: Some(build_expected_violation_with_layers(
+                CheckerType::Layer, false, "product", "utilities")),
         };
         test_check(&checker_with_layers(), &mut test_checker)
     }
@@ -228,9 +222,8 @@ mod tests {
                 layer: Some("utilities".to_string()),
                 ..default_referencing_pack()
             },
-            expected_violation: Some(build_expected_violation(
-                "Layer violation: `::Bar` belongs to `packs/bar` (whose layer is `product`) cannot be accessed from `packs/foo` (whose layer is `utilities`)".to_string(),
-                "layer".to_string(), true)),
+            expected_violation: Some(build_expected_violation_with_layers(
+                CheckerType::Layer, true, "product", "utilities")),
         };
         test_check(&checker_with_layers(), &mut test_checker)
     }