From 64da47fccce11a4b402a76964b716aadcac65390 Mon Sep 17 00:00:00 2001 From: Martin Emde Date: Tue, 6 Jan 2026 16:11:28 -0800 Subject: [PATCH 1/5] Add --color option to control terminal colors --- src/packs.rs | 26 +++++++++++++- src/packs/checker/common_test.rs | 16 +-------- src/packs/checker/output_helper.rs | 2 +- src/packs/checker/pack_checker.rs | 10 +++--- src/packs/cli.rs | 24 +++++++++++-- tests/check_test.rs | 58 ++++++++++++++++++++++++++++++ 6 files changed, 112 insertions(+), 24 deletions(-) diff --git a/src/packs.rs b/src/packs.rs index 3cc16a9..6b909c5 100644 --- a/src/packs.rs +++ b/src/packs.rs @@ -40,6 +40,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; @@ -47,8 +48,10 @@ pub(crate) use package_todo::PackageTodo; // External imports use anyhow::Context; +use regex::Regex; use serde::Deserialize; use serde::Serialize; +use std::io::IsTerminal; use std::path::PathBuf; pub fn greet() { @@ -70,9 +73,25 @@ pub fn create( Ok(()) } +/// Colorize file:line:column patterns for terminal output +fn colorize_output(s: &str) -> String { + let re = Regex::new(r"(?m)^(.+:\d+:\d+)$").unwrap(); + re.replace_all(s, "\x1b[36m$1\x1b[0m").to_string() +} + +/// Determine whether to use colors based on the color choice +fn should_colorize(color: ColorChoice) -> bool { + match color { + ColorChoice::Always => true, + ColorChoice::Never => false, + ColorChoice::Auto => std::io::stdout().is_terminal(), + } +} + pub fn check( configuration: &Configuration, output_format: OutputFormat, + color: ColorChoice, files: Vec, ) -> anyhow::Result<()> { let result = checker::check_all(configuration, files) @@ -80,7 +99,12 @@ pub fn check( match output_format { OutputFormat::Packwerk => { - println!("{}", result); + let output = format!("{}", result); + if should_colorize(color) { + println!("{}", colorize_output(&output)); + } else { + println!("{}", output); + } } OutputFormat::CSV => { csv::write_csv(&result, std::io::stdout())?; diff --git a/src/packs/checker/common_test.rs b/src/packs/checker/common_test.rs index f090f70..3a1d69d 100644 --- a/src/packs/checker/common_test.rs +++ b/src/packs/checker/common_test.rs @@ -138,21 +138,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(()) } diff --git a/src/packs/checker/output_helper.rs b/src/packs/checker/output_helper.rs index 8da5d15..63eda7a 100644 --- a/src/packs/checker/output_helper.rs +++ b/src/packs/checker/output_helper.rs @@ -2,7 +2,7 @@ use super::reference::Reference; pub fn print_reference_location(reference: &Reference) -> String { format!( - "\x1b[36m{}\x1b[0m:{}:{}\n", + "{}:{}:{}\n", reference.relative_referencing_file, reference.source_location.line, reference.source_location.column, diff --git a/src/packs/checker/pack_checker.rs b/src/packs/checker/pack_checker.rs index d2eaed4..6918882 100644 --- a/src/packs/checker/pack_checker.rs +++ b/src/packs/checker/pack_checker.rs @@ -305,7 +305,7 @@ mod tests { assert_eq!(checker.referencing_pack_name(), "packs/baz".to_string()); assert_eq!(checker.rules_checker_setting(), &CheckerSetting::False); assert!(!checker.violation_globally_disabled()); - let expected_violation_message: String = "\u{1b}[36mpacks/baz/public/baz.rb\u{1b}[0m:3:4\nFolder Privacy violation: `Foo` belongs to `packs/foo`, which is private to `packs/baz` as it is not a sibling pack or parent pack.".into(); + let expected_violation_message: String = "packs/baz/public/baz.rb:3:4\nFolder Privacy violation: `Foo` belongs to `packs/foo`, which is private to `packs/baz` as it is not a sibling pack or parent pack.".into(); assert_eq!( checker.interpolate_violation_message(None), expected_violation_message @@ -335,7 +335,7 @@ mod tests { let checker = PackChecker::new(&config, CheckerType::Privacy, &refer)?; assert_eq!(checker.violation_direction(), ViolationDirection::Incoming); - let expected_violation_message: String = "\u{1b}[36mpacks/baz/public/baz.rb\u{1b}[0m:3:4\nPrivacy violation: `Foo` is private to `packs/foo`, but referenced from `packs/baz`".into(); + let expected_violation_message: String = "packs/baz/public/baz.rb:3:4\nPrivacy violation: `Foo` is private to `packs/foo`, but referenced from `packs/baz`".into(); assert_eq!( checker.interpolate_violation_message(None), expected_violation_message @@ -351,7 +351,7 @@ mod tests { PackChecker::new(&config, CheckerType::Dependency, &refer)?; assert_eq!(checker.violation_direction(), ViolationDirection::Outgoing); - let expected_violation_message: String = "\u{1b}[36mpacks/baz/public/baz.rb\u{1b}[0m:3:4\nDependency violation: `Foo` belongs to `packs/foo`, but `package.yml` does not specify a dependency on `packs/foo`.".into(); + let expected_violation_message: String = "packs/baz/public/baz.rb:3:4\nDependency violation: `Foo` belongs to `packs/foo`, but `package.yml` does not specify a dependency on `packs/foo`.".into(); assert_eq!( checker.interpolate_violation_message(None), expected_violation_message @@ -366,7 +366,7 @@ mod tests { let checker = PackChecker::new(&config, CheckerType::Layer, &refer)?; assert_eq!(checker.violation_direction(), ViolationDirection::Outgoing); - let expected_violation_message: String = "\u{1b}[36mpacks/baz/public/baz.rb\u{1b}[0m:3:4\nLayer violation: `Foo` belongs to `packs/foo` (whose layer is `{{defining_layer}}`) cannot be accessed from `packs/baz` (whose layer is `{{referencing_layer}}`)".into(); + let expected_violation_message: String = "packs/baz/public/baz.rb:3:4\nLayer violation: `Foo` belongs to `packs/foo` (whose layer is `{{defining_layer}}`) cannot be accessed from `packs/baz` (whose layer is `{{referencing_layer}}`)".into(); assert_eq!( checker.interpolate_violation_message(None), expected_violation_message @@ -382,7 +382,7 @@ mod tests { PackChecker::new(&config, CheckerType::Visibility, &refer)?; assert_eq!(checker.violation_direction(), ViolationDirection::Incoming); - let expected_violation_message: String = "\u{1b}[36mpacks/baz/public/baz.rb\u{1b}[0m:3:4\nVisibility violation: `Foo` belongs to `packs/foo`, which is not visible to `packs/baz`".into(); + let expected_violation_message: String = "packs/baz/public/baz.rb:3:4\nVisibility violation: `Foo` belongs to `packs/foo`, which is not visible to `packs/baz`".into(); assert_eq!( checker.interpolate_violation_message(None), expected_violation_message diff --git a/src/packs/cli.rs b/src/packs/cli.rs index ff2e84e..7501180 100644 --- a/src/packs/cli.rs +++ b/src/packs/cli.rs @@ -70,6 +70,10 @@ struct Args { /// Globally disable enforce_visibility #[arg(long)] disable_enforce_visibility: bool, + + /// When to use colors in output + #[arg(long, value_enum, default_value_t = ColorChoice::Auto, global = true)] + color: ColorChoice, } #[derive(Subcommand, Debug)] @@ -178,6 +182,17 @@ pub enum OutputFormat { JSON, } +#[derive(ValueEnum, Copy, Clone, Debug, PartialEq, Eq, Default)] +pub enum ColorChoice { + /// Always use colors + Always, + /// Never use colors + Never, + /// Use colors when outputting to a terminal (default) + #[default] + Auto, +} + #[derive(Debug, Args)] struct ListDefinitionsArgs { /// Show constants with multiple definitions only @@ -273,7 +288,7 @@ pub fn run() -> anyhow::Result<()> { } => { configuration.ignore_recorded_violations = ignore_recorded_violations; - packs::check(&configuration, output_format, files) + packs::check(&configuration, output_format, args.color, files) } Command::CheckContents { ignore_recorded_violations, @@ -284,7 +299,12 @@ pub fn run() -> anyhow::Result<()> { let absolute_path = get_absolute_path(file.clone(), &configuration); configuration.stdin_file_path = Some(absolute_path); - packs::check(&configuration, OutputFormat::Packwerk, vec![file]) + packs::check( + &configuration, + OutputFormat::Packwerk, + args.color, + vec![file], + ) } Command::Update => packs::update(&configuration), Command::Validate => { diff --git a/tests/check_test.rs b/tests/check_test.rs index 804e8e3..4f0d043 100644 --- a/tests/check_test.rs +++ b/tests/check_test.rs @@ -644,3 +644,61 @@ fn test_check_with_invalid_output_format() -> Result<(), Box> { Ok(()) } + +#[test] +fn test_check_with_color_always() -> Result<(), Box> { + let output = cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/simple_app") + .arg("--color=always") + .arg("check") + .assert() + .code(1) + .get_output() + .stdout + .clone(); + + let raw_output = String::from_utf8_lossy(&output).to_string(); + + // With --color=always, output should contain ANSI color codes + assert!( + raw_output.contains("\x1b[36m"), + "Output should contain cyan color code with --color=always" + ); + assert!( + raw_output.contains("\x1b[0m"), + "Output should contain reset color code with --color=always" + ); + + common::teardown(); + Ok(()) +} + +#[test] +fn test_check_with_color_never() -> Result<(), Box> { + let output = cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/simple_app") + .arg("--color=never") + .arg("check") + .assert() + .code(1) + .get_output() + .stdout + .clone(); + + let raw_output = String::from_utf8_lossy(&output).to_string(); + + // With --color=never, output should not contain ANSI color codes + assert!( + !raw_output.contains("\x1b["), + "Output should not contain ANSI escape codes with --color=never" + ); + + // But should still contain the violation content + assert!(raw_output.contains("2 violation(s) detected:")); + assert!(raw_output.contains("Dependency violation")); + + common::teardown(); + Ok(()) +} From 4fd1f6f7e9b8273ad6e5ab960c0e2e33580fc0b1 Mon Sep 17 00:00:00 2001 From: Martin Emde Date: Tue, 6 Jan 2026 16:49:13 -0800 Subject: [PATCH 2/5] Refactor all text output to use formatters --- src/packs.rs | 19 ++--- src/packs/text.rs | 174 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 180 insertions(+), 13 deletions(-) create mode 100644 src/packs/text.rs diff --git a/src/packs.rs b/src/packs.rs index 6b909c5..64b4666 100644 --- a/src/packs.rs +++ b/src/packs.rs @@ -18,6 +18,7 @@ pub(crate) mod monkey_patch_detection; pub(crate) mod pack; pub(crate) mod parsing; pub(crate) mod raw_configuration; +pub(crate) mod text; pub(crate) mod walk_directory; mod constant_dependencies; @@ -48,7 +49,6 @@ pub(crate) use package_todo::PackageTodo; // External imports use anyhow::Context; -use regex::Regex; use serde::Deserialize; use serde::Serialize; use std::io::IsTerminal; @@ -73,12 +73,6 @@ pub fn create( Ok(()) } -/// Colorize file:line:column patterns for terminal output -fn colorize_output(s: &str) -> String { - let re = Regex::new(r"(?m)^(.+:\d+:\d+)$").unwrap(); - re.replace_all(s, "\x1b[36m$1\x1b[0m").to_string() -} - /// Determine whether to use colors based on the color choice fn should_colorize(color: ColorChoice) -> bool { match color { @@ -99,12 +93,11 @@ pub fn check( match output_format { OutputFormat::Packwerk => { - let output = format!("{}", result); - if should_colorize(color) { - println!("{}", colorize_output(&output)); - } else { - println!("{}", output); - } + text::write_text( + &result, + std::io::stdout(), + should_colorize(color), + )?; } OutputFormat::CSV => { csv::write_csv(&result, std::io::stdout())?; diff --git a/src/packs/text.rs b/src/packs/text.rs new file mode 100644 index 0000000..e1b5fa4 --- /dev/null +++ b/src/packs/text.rs @@ -0,0 +1,174 @@ +//! Text output formatter for `pks check`. +//! +//! Formats check results as human-readable text with optional color output. + +use super::bin_locater; +use super::checker::{ + build_strict_violation_message, CheckAllResult, Violation, +}; + +/// Format a file:line:column location, optionally with color +fn format_location( + file: &str, + line: usize, + column: usize, + colorize: bool, +) -> String { + if colorize { + format!("\x1b[36m{}:{}:{}\x1b[0m", file, line, column) + } else { + format!("{}:{}:{}", file, line, column) + } +} + +/// Format a violation message with optional colorization of the location +fn format_violation_message(violation: &Violation, colorize: bool) -> String { + let location = format_location( + &violation.identifier.file, + violation.source_location.line, + violation.source_location.column, + colorize, + ); + + // The message starts with "file:line:column\n" - replace it with our formatted version + let message_without_location = violation + .message + .split_once('\n') + .map(|(_, rest)| rest) + .unwrap_or(&violation.message); + + format!("{}\n{}", location, message_without_location) +} + +pub fn write_text( + result: &CheckAllResult, + mut writer: W, + colorize: bool, +) -> anyhow::Result<()> { + if !result.has_violations() { + writeln!(writer, "No violations detected!")?; + return Ok(()); + } + + if !result.reportable_violations.is_empty() { + let mut sorted_violations: Vec<&Violation> = + result.reportable_violations.iter().collect(); + sorted_violations.sort_by(|a, b| a.message.cmp(&b.message)); + + writeln!(writer, "{} violation(s) detected:", sorted_violations.len())?; + + for violation in sorted_violations { + let formatted = format_violation_message(violation, colorize); + writeln!(writer, "{}\n", formatted)?; + } + } + + if !result.stale_violations.is_empty() { + writeln!( + writer, + "There were stale violations found, please run `{} update`", + bin_locater::packs_bin_name(), + )?; + } + + if !result.strict_mode_violations.is_empty() { + for v in result.strict_mode_violations.iter() { + let error_message = build_strict_violation_message(&v.identifier); + writeln!(writer, "{}", error_message)?; + } + } + + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::packs::checker::{Violation, ViolationIdentifier}; + use crate::packs::SourceLocation; + use std::collections::HashSet; + + fn sample_violation() -> Violation { + Violation { + message: + "foo/bar/file.rb:10:5\nPrivacy violation: `Foo` is private" + .to_string(), + identifier: ViolationIdentifier { + violation_type: "Privacy".to_string(), + strict: false, + file: "foo/bar/file.rb".to_string(), + constant_name: "Foo".to_string(), + referencing_pack_name: "bar".to_string(), + defining_pack_name: "foo".to_string(), + }, + source_location: SourceLocation { + line: 10, + column: 5, + }, + } + } + + #[test] + fn test_format_location_with_color() { + let result = format_location("foo.rb", 10, 5, true); + assert_eq!(result, "\x1b[36mfoo.rb:10:5\x1b[0m"); + } + + #[test] + fn test_format_location_without_color() { + let result = format_location("foo.rb", 10, 5, false); + assert_eq!(result, "foo.rb:10:5"); + } + + #[test] + fn test_format_violation_message_with_color() { + let violation = sample_violation(); + let result = format_violation_message(&violation, true); + assert_eq!( + result, + "\x1b[36mfoo/bar/file.rb:10:5\x1b[0m\nPrivacy violation: `Foo` is private" + ); + } + + #[test] + fn test_format_violation_message_without_color() { + let violation = sample_violation(); + let result = format_violation_message(&violation, false); + assert_eq!( + result, + "foo/bar/file.rb:10:5\nPrivacy violation: `Foo` is private" + ); + } + + #[test] + fn test_write_text_no_violations() { + let result = CheckAllResult { + reportable_violations: HashSet::new(), + stale_violations: Vec::new(), + strict_mode_violations: HashSet::new(), + }; + + let mut output = Vec::new(); + write_text(&result, &mut output, false).unwrap(); + assert_eq!( + String::from_utf8(output).unwrap(), + "No violations detected!\n" + ); + } + + #[test] + fn test_write_text_with_violations() { + let result = CheckAllResult { + reportable_violations: [sample_violation()].into_iter().collect(), + stale_violations: Vec::new(), + strict_mode_violations: HashSet::new(), + }; + + let mut output = Vec::new(); + write_text(&result, &mut output, false).unwrap(); + let text = String::from_utf8(output).unwrap(); + assert!(text.contains("1 violation(s) detected:")); + assert!(text.contains("foo/bar/file.rb:10:5")); + assert!(text.contains("Privacy violation: `Foo` is private")); + } +} From c7848fa230b7b8c40d1a7f1a3645bbe9613c6588 Mon Sep 17 00:00:00 2001 From: Martin Emde Date: Tue, 6 Jan 2026 20:58:00 -0800 Subject: [PATCH 3/5] Refactor to move formatting to the edge --- src/packs/checker.rs | 44 ++++++++++++++---- src/packs/checker/dependency.rs | 4 +- src/packs/checker/folder_privacy.rs | 4 +- src/packs/checker/layer.rs | 4 +- src/packs/checker/output_helper.rs | 10 ----- src/packs/checker/pack_checker.rs | 23 +++++----- src/packs/checker/privacy.rs | 6 +-- src/packs/checker/visibility.rs | 4 +- src/packs/checker_configuration.rs | 10 ++--- src/packs/csv.rs | 12 ++++- src/packs/json.rs | 2 + src/packs/text.rs | 69 ++++++++++++++++++++++++----- 12 files changed, 131 insertions(+), 61 deletions(-) delete mode 100644 src/packs/checker/output_helper.rs diff --git a/src/packs/checker.rs b/src/packs/checker.rs index 58544af..ddd56fd 100644 --- a/src/packs/checker.rs +++ b/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; @@ -52,6 +51,9 @@ 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 +/// +/// `message` contains the violation description without the file location prefix. +/// Formatters are responsible for combining source_location with message as needed. #[derive(PartialEq, Clone, Eq, Hash, Debug)] pub struct Violation { pub message: String, @@ -80,6 +82,8 @@ pub struct CheckAllResult { pub strict_mode_violations: HashSet, } +const REFERENCE_LOCATION_PLACEHOLDER: &str = "{{reference_location}}"; + impl CheckAllResult { pub fn has_violations(&self) -> bool { !self.reportable_violations.is_empty() @@ -87,6 +91,27 @@ impl CheckAllResult { || !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> = @@ -96,7 +121,8 @@ impl CheckAllResult { writeln!(f, "{} violation(s) detected:", sorted_violations.len())?; for violation in sorted_violations { - writeln!(f, "{}\n", violation.message)?; + let formatted = Self::format_violation_message(violation); + writeln!(f, "{}\n", formatted)?; } } @@ -528,9 +554,9 @@ mod tests { #[test] fn test_write_violations() { - let chec_result = CheckAllResult { + let check_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(), + message: "Privacy violation: `::Foo::PrivateClass` is private to `foo`, but referenced from `bar`".to_string(), identifier: ViolationIdentifier { violation_type: "Privacy".to_string(), strict: false, @@ -542,7 +568,7 @@ mod tests { 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(), + message: "Dependency violation: `::Foo::AnotherClass` is not allowed to depend on `::Bar::SomeClass`".to_string(), identifier: ViolationIdentifier { violation_type: "Dependency".to_string(), strict: false, @@ -558,15 +584,15 @@ mod tests { }; 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` +foo/bar/file1.rb:10:5 +Privacy violation: `::Foo::PrivateClass` is private to `foo`, but referenced from `bar` + "; - let actual = format!("{}", chec_result); + let actual = format!("{}", check_result); assert_eq!(actual, expected_output); } diff --git a/src/packs/checker/dependency.rs b/src/packs/checker/dependency.rs index 095d110..2d76090 100644 --- a/src/packs/checker/dependency.rs +++ b/src/packs/checker/dependency.rs @@ -225,7 +225,7 @@ mod tests { enforce_dependencies: Some(CheckerSetting::True), ..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 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)), }; test_check( @@ -253,7 +253,7 @@ mod tests { enforce_dependencies: Some(CheckerSetting::Strict), ..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 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)), }; test_check( diff --git a/src/packs/checker/folder_privacy.rs b/src/packs/checker/folder_privacy.rs index b0e8749..a5a9edf 100644 --- a/src/packs/checker/folder_privacy.rs +++ b/src/packs/checker/folder_privacy.rs @@ -89,7 +89,7 @@ mod tests { relative_path: PathBuf::from("packs/foo"), ..default_referencing_pack()}, expected_violation: Some(build_expected_violation( - "packs/foo/app/services/foo.rb:3:1\nFolder 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 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)), }; test_check( @@ -154,7 +154,7 @@ mod tests { relative_path: PathBuf::from("packs/foo"), ..default_referencing_pack()}, expected_violation: Some(build_expected_violation( - "packs/foo/app/services/foo.rb:3:1\nFolder 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 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)), }; test_check( diff --git a/src/packs/checker/layer.rs b/src/packs/checker/layer.rs index a9d200f..bdc1e30 100644 --- a/src/packs/checker/layer.rs +++ b/src/packs/checker/layer.rs @@ -205,7 +205,7 @@ mod tests { ..default_referencing_pack() }, expected_violation: Some(build_expected_violation( - "packs/foo/app/services/foo.rb:3:1\nLayer violation: `::Bar` belongs to `packs/bar` (whose layer is `product`) cannot be accessed from `packs/foo` (whose layer is `utilities`)".to_string(), + "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)), }; test_check(&checker_with_layers(), &mut test_checker) @@ -229,7 +229,7 @@ mod tests { ..default_referencing_pack() }, expected_violation: Some(build_expected_violation( - "packs/foo/app/services/foo.rb:3:1\nLayer violation: `::Bar` belongs to `packs/bar` (whose layer is `product`) cannot be accessed from `packs/foo` (whose layer is `utilities`)".to_string(), + "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)), }; test_check(&checker_with_layers(), &mut test_checker) diff --git a/src/packs/checker/output_helper.rs b/src/packs/checker/output_helper.rs deleted file mode 100644 index 63eda7a..0000000 --- a/src/packs/checker/output_helper.rs +++ /dev/null @@ -1,10 +0,0 @@ -use super::reference::Reference; - -pub fn print_reference_location(reference: &Reference) -> String { - format!( - "{}:{}:{}\n", - reference.relative_referencing_file, - reference.source_location.line, - reference.source_location.column, - ) -} diff --git a/src/packs/checker/pack_checker.rs b/src/packs/checker/pack_checker.rs index 6918882..5cb732b 100644 --- a/src/packs/checker/pack_checker.rs +++ b/src/packs/checker/pack_checker.rs @@ -6,10 +6,7 @@ use crate::packs::{ Configuration, }; -use super::{ - output_helper::print_reference_location, reference::Reference, Violation, - ViolationIdentifier, -}; +use super::{reference::Reference, Violation, ViolationIdentifier}; pub struct PackChecker<'a> { pub configuration: &'a Configuration, @@ -195,10 +192,6 @@ impl<'a> PackChecker<'a> { self.defining_pack.unwrap().name.clone(), ); map.insert("{{constant_name}}", self.reference.constant_name.clone()); - map.insert( - "{{reference_location}}", - print_reference_location(self.reference), - ); map.insert( "{{referencing_pack_relative_yml}}", self.referencing_pack @@ -206,6 +199,10 @@ impl<'a> PackChecker<'a> { .to_string_lossy() .to_string(), ); + // NOTE: {{reference_location}} is NOT substituted here. + // Formatters are responsible for substituting it with their preferred format + // (colorized for text output, etc.). This allows custom templates to use + // {{reference_location}} while giving formatters control over presentation. let mut interpolated_msg = self.checker_configuration.checker_error_template(); @@ -305,7 +302,7 @@ mod tests { assert_eq!(checker.referencing_pack_name(), "packs/baz".to_string()); assert_eq!(checker.rules_checker_setting(), &CheckerSetting::False); assert!(!checker.violation_globally_disabled()); - let expected_violation_message: String = "packs/baz/public/baz.rb:3:4\nFolder Privacy violation: `Foo` belongs to `packs/foo`, which is private to `packs/baz` as it is not a sibling pack or parent pack.".into(); + let expected_violation_message: String = "Folder Privacy violation: `Foo` belongs to `packs/foo`, which is private to `packs/baz` as it is not a sibling pack or parent pack.".into(); assert_eq!( checker.interpolate_violation_message(None), expected_violation_message @@ -335,7 +332,7 @@ mod tests { let checker = PackChecker::new(&config, CheckerType::Privacy, &refer)?; assert_eq!(checker.violation_direction(), ViolationDirection::Incoming); - let expected_violation_message: String = "packs/baz/public/baz.rb:3:4\nPrivacy violation: `Foo` is private to `packs/foo`, but referenced from `packs/baz`".into(); + let expected_violation_message: String = "Privacy violation: `Foo` is private to `packs/foo`, but referenced from `packs/baz`".into(); assert_eq!( checker.interpolate_violation_message(None), expected_violation_message @@ -351,7 +348,7 @@ mod tests { PackChecker::new(&config, CheckerType::Dependency, &refer)?; assert_eq!(checker.violation_direction(), ViolationDirection::Outgoing); - let expected_violation_message: String = "packs/baz/public/baz.rb:3:4\nDependency violation: `Foo` belongs to `packs/foo`, but `package.yml` does not specify a dependency on `packs/foo`.".into(); + let expected_violation_message: String = "Dependency violation: `Foo` belongs to `packs/foo`, but `package.yml` does not specify a dependency on `packs/foo`.".into(); assert_eq!( checker.interpolate_violation_message(None), expected_violation_message @@ -366,7 +363,7 @@ mod tests { let checker = PackChecker::new(&config, CheckerType::Layer, &refer)?; assert_eq!(checker.violation_direction(), ViolationDirection::Outgoing); - let expected_violation_message: String = "packs/baz/public/baz.rb:3:4\nLayer violation: `Foo` belongs to `packs/foo` (whose layer is `{{defining_layer}}`) cannot be accessed from `packs/baz` (whose layer is `{{referencing_layer}}`)".into(); + let expected_violation_message: String = "Layer violation: `Foo` belongs to `packs/foo` (whose layer is `{{defining_layer}}`) cannot be accessed from `packs/baz` (whose layer is `{{referencing_layer}}`)".into(); assert_eq!( checker.interpolate_violation_message(None), expected_violation_message @@ -382,7 +379,7 @@ mod tests { PackChecker::new(&config, CheckerType::Visibility, &refer)?; assert_eq!(checker.violation_direction(), ViolationDirection::Incoming); - let expected_violation_message: String = "packs/baz/public/baz.rb:3:4\nVisibility violation: `Foo` belongs to `packs/foo`, which is not visible to `packs/baz`".into(); + let expected_violation_message: String = "Visibility violation: `Foo` belongs to `packs/foo`, which is not visible to `packs/baz`".into(); assert_eq!( checker.interpolate_violation_message(None), expected_violation_message diff --git a/src/packs/checker/privacy.rs b/src/packs/checker/privacy.rs index 8394421..56b5186 100644 --- a/src/packs/checker/privacy.rs +++ b/src/packs/checker/privacy.rs @@ -171,7 +171,7 @@ mod tests { }), referencing_pack: default_referencing_pack(), expected_violation: Some(build_expected_violation( - String::from("packs/foo/app/services/foo.rb:3:1\nPrivacy violation: `::Bar` is private to `packs/bar`, but referenced from `packs/foo`"), + String::from("Privacy violation: `::Bar` is private to `packs/bar`, but referenced from `packs/foo`"), String::from("privacy"), false, )), }; @@ -199,7 +199,7 @@ mod tests { }), referencing_pack: default_referencing_pack(), expected_violation: Some(build_expected_violation( - String::from("packs/foo/app/services/foo.rb:3:1\nPrivacy violation: `::Bar` is private to `packs/bar`, but referenced from `packs/foo`"), + String::from("Privacy violation: `::Bar` is private to `packs/bar`, but referenced from `packs/foo`"), String::from("privacy"), true, )), }; @@ -453,7 +453,7 @@ mod tests { }), referencing_pack: default_referencing_pack(), expected_violation: Some(build_expected_violation_with_constant( - String::from("packs/foo/app/services/foo.rb:3:1\nPrivacy violation: `::Bar::BarChild` is private to `packs/bar`, but referenced from `packs/foo`"), + String::from("Privacy violation: `::Bar::BarChild` is private to `packs/bar`, but referenced from `packs/foo`"), String::from("privacy"), false, String::from("::Bar::BarChild") )), diff --git a/src/packs/checker/visibility.rs b/src/packs/checker/visibility.rs index 2562050..a0be4dc 100644 --- a/src/packs/checker/visibility.rs +++ b/src/packs/checker/visibility.rs @@ -107,7 +107,7 @@ mod tests { relative_path: PathBuf::from("packs/foo"), ..default_referencing_pack()}, expected_violation: Some(build_expected_violation( - "packs/foo/app/services/foo.rb:3:1\nVisibility violation: `::Bar` belongs to `packs/bar`, which is not visible to `packs/foo`".to_string(), + "Visibility violation: `::Bar` belongs to `packs/bar`, which is not visible to `packs/foo`".to_string(), "visibility".to_string(), false)), }; test_check( @@ -173,7 +173,7 @@ mod tests { relative_path: PathBuf::from("packs/foo"), ..default_referencing_pack()}, expected_violation: Some(build_expected_violation( - "packs/foo/app/services/foo.rb:3:1\nVisibility violation: `::Bar` belongs to `packs/bar`, which is not visible to `packs/foo`".to_string(), + "Visibility violation: `::Bar` belongs to `packs/bar`, which is not visible to `packs/foo`".to_string(), "visibility".to_string(), true)), }; test_check( diff --git a/src/packs/checker_configuration.rs b/src/packs/checker_configuration.rs index cf11da4..2117d37 100644 --- a/src/packs/checker_configuration.rs +++ b/src/packs/checker_configuration.rs @@ -13,11 +13,11 @@ pub struct CheckerConfiguration { pub override_error_template: Option, } -const DEFAULT_DEPENDENCY_TEMPLATE: &str = "{{reference_location}}Dependency violation: `{{constant_name}}` belongs to `{{defining_pack_name}}`, but `{{referencing_pack_relative_yml}}` does not specify a dependency on `{{defining_pack_name}}`."; -const DEFAULT_FOLDER_PRIVACY_TEMPLATE: &str = "{{reference_location}}{{violation_name}} violation: `{{constant_name}}` belongs to `{{defining_pack_name}}`, which is private to `{{referencing_pack_name}}` as it is not a sibling pack or parent pack."; -const DEFAULT_LAYER_TEMPLATE: &str = "{{reference_location}}Layer violation: `{{constant_name}}` belongs to `{{defining_pack_name}}` (whose layer is `{{defining_layer}}`) cannot be accessed from `{{referencing_pack_name}}` (whose layer is `{{referencing_layer}}`)"; -const DEFAULT_VISIBILITY_TEMPLATE: &str = "{{reference_location}}Visibility violation: `{{constant_name}}` belongs to `{{defining_pack_name}}`, which is not visible to `{{referencing_pack_name}}`"; -const DEFAULT_PRIVACY_TEMPLATE: &str = "{{reference_location}}Privacy violation: `{{constant_name}}` is private to `{{defining_pack_name}}`, but referenced from `{{referencing_pack_name}}`"; +const DEFAULT_DEPENDENCY_TEMPLATE: &str = "Dependency violation: `{{constant_name}}` belongs to `{{defining_pack_name}}`, but `{{referencing_pack_relative_yml}}` does not specify a dependency on `{{defining_pack_name}}`."; +const DEFAULT_FOLDER_PRIVACY_TEMPLATE: &str = "{{violation_name}} violation: `{{constant_name}}` belongs to `{{defining_pack_name}}`, which is private to `{{referencing_pack_name}}` as it is not a sibling pack or parent pack."; +const DEFAULT_LAYER_TEMPLATE: &str = "Layer violation: `{{constant_name}}` belongs to `{{defining_pack_name}}` (whose layer is `{{defining_layer}}`) cannot be accessed from `{{referencing_pack_name}}` (whose layer is `{{referencing_layer}}`)"; +const DEFAULT_VISIBILITY_TEMPLATE: &str = "Visibility violation: `{{constant_name}}` belongs to `{{defining_pack_name}}`, which is not visible to `{{referencing_pack_name}}`"; +const DEFAULT_PRIVACY_TEMPLATE: &str = "Privacy violation: `{{constant_name}}` is private to `{{defining_pack_name}}`, but referenced from `{{referencing_pack_name}}`"; impl CheckerConfiguration { pub fn new(checker_type: CheckerType) -> Self { diff --git a/src/packs/csv.rs b/src/packs/csv.rs index d1739b8..0c8dbb8 100644 --- a/src/packs/csv.rs +++ b/src/packs/csv.rs @@ -2,6 +2,16 @@ use itertools::chain; use super::checker::{build_strict_violation_message, CheckAllResult}; +fn format_message_with_location(v: &super::checker::Violation) -> String { + format!( + "{}:{}:{}\n{}", + v.identifier.file, + v.source_location.line, + v.source_location.column, + v.message + ) +} + pub fn write_csv( result: &CheckAllResult, writer: W, @@ -30,7 +40,7 @@ pub fn write_csv( let message = if violation.identifier.strict { build_strict_violation_message(&violation.identifier) } else { - violation.message.to_string() + format_message_with_location(violation) }; wtr.serialize(( &identifier.violation_type, diff --git a/src/packs/json.rs b/src/packs/json.rs index c9599a9..9239746 100644 --- a/src/packs/json.rs +++ b/src/packs/json.rs @@ -54,6 +54,8 @@ pub fn write_json( &result.strict_mode_violations ); + // JSON outputs raw structured data - consumers can format as needed. + // Location is provided as separate file/line/column fields. let violations: Vec = all_violations .map(|v| { let message = if v.identifier.strict { diff --git a/src/packs/text.rs b/src/packs/text.rs index e1b5fa4..6487cd1 100644 --- a/src/packs/text.rs +++ b/src/packs/text.rs @@ -21,7 +21,13 @@ fn format_location( } } -/// Format a violation message with optional colorization of the location +const REFERENCE_LOCATION_PLACEHOLDER: &str = "{{reference_location}}"; + +/// Format a violation message with optional colorization of the location. +/// +/// This function is responsible for substituting `{{reference_location}}` in custom templates. +/// - If the message contains `{{reference_location}}`, substitute it with the formatted location +/// - Otherwise, prepend the location on its own line (default behavior) fn format_violation_message(violation: &Violation, colorize: bool) -> String { let location = format_location( &violation.identifier.file, @@ -30,14 +36,15 @@ fn format_violation_message(violation: &Violation, colorize: bool) -> String { colorize, ); - // The message starts with "file:line:column\n" - replace it with our formatted version - let message_without_location = violation - .message - .split_once('\n') - .map(|(_, rest)| rest) - .unwrap_or(&violation.message); - - format!("{}\n{}", location, message_without_location) + 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) + } } pub fn write_text( @@ -90,9 +97,7 @@ mod tests { fn sample_violation() -> Violation { Violation { - message: - "foo/bar/file.rb:10:5\nPrivacy violation: `Foo` is private" - .to_string(), + message: "Privacy violation: `Foo` is private".to_string(), identifier: ViolationIdentifier { violation_type: "Privacy".to_string(), strict: false, @@ -171,4 +176,44 @@ mod tests { assert!(text.contains("foo/bar/file.rb:10:5")); assert!(text.contains("Privacy violation: `Foo` is private")); } + + fn custom_template_violation() -> Violation { + Violation { + // Message with {{reference_location}} placeholder (from custom template) + message: "{{reference_location}}Custom privacy error for `Foo`" + .to_string(), + identifier: ViolationIdentifier { + violation_type: "Privacy".to_string(), + strict: false, + file: "foo/bar/file.rb".to_string(), + constant_name: "Foo".to_string(), + referencing_pack_name: "bar".to_string(), + defining_pack_name: "foo".to_string(), + }, + source_location: SourceLocation { + line: 10, + column: 5, + }, + } + } + + #[test] + fn test_format_violation_message_with_custom_template_no_color() { + let violation = custom_template_violation(); + let result = format_violation_message(&violation, false); + assert_eq!( + result, + "foo/bar/file.rb:10:5\nCustom privacy error for `Foo`" + ); + } + + #[test] + fn test_format_violation_message_with_custom_template_with_color() { + let violation = custom_template_violation(); + let result = format_violation_message(&violation, true); + assert_eq!( + result, + "\x1b[36mfoo/bar/file.rb:10:5\x1b[0m\nCustom privacy error for `Foo`" + ); + } } From ae124b3fe990e455a49960f620edd621d3a35542 Mon Sep 17 00:00:00 2001 From: Martin Emde Date: Tue, 6 Jan 2026 21:27:16 -0800 Subject: [PATCH 4/5] Use an enum for color mode --- src/packs.rs | 16 +++++++++++----- src/packs/text.rs | 45 ++++++++++++++++++++++++++++----------------- 2 files changed, 39 insertions(+), 22 deletions(-) diff --git a/src/packs.rs b/src/packs.rs index 64b4666..84bedcc 100644 --- a/src/packs.rs +++ b/src/packs.rs @@ -74,11 +74,17 @@ pub fn create( } /// Determine whether to use colors based on the color choice -fn should_colorize(color: ColorChoice) -> bool { +fn color_mode_for(color: ColorChoice) -> text::ColorMode { match color { - ColorChoice::Always => true, - ColorChoice::Never => false, - ColorChoice::Auto => std::io::stdout().is_terminal(), + 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 + } + } } } @@ -96,7 +102,7 @@ pub fn check( text::write_text( &result, std::io::stdout(), - should_colorize(color), + color_mode_for(color), )?; } OutputFormat::CSV => { diff --git a/src/packs/text.rs b/src/packs/text.rs index 6487cd1..cf27463 100644 --- a/src/packs/text.rs +++ b/src/packs/text.rs @@ -3,6 +3,13 @@ //! Formats check results as human-readable text with optional color output. use super::bin_locater; + +/// Controls whether output should include ANSI color codes. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ColorMode { + Colored, + Plain, +} use super::checker::{ build_strict_violation_message, CheckAllResult, Violation, }; @@ -12,12 +19,13 @@ fn format_location( file: &str, line: usize, column: usize, - colorize: bool, + color_mode: ColorMode, ) -> String { - if colorize { - format!("\x1b[36m{}:{}:{}\x1b[0m", file, line, column) - } else { - format!("{}:{}:{}", file, line, column) + match color_mode { + ColorMode::Colored => { + format!("\x1b[36m{}:{}:{}\x1b[0m", file, line, column) + } + ColorMode::Plain => format!("{}:{}:{}", file, line, column), } } @@ -28,12 +36,15 @@ const REFERENCE_LOCATION_PLACEHOLDER: &str = "{{reference_location}}"; /// This function is responsible for substituting `{{reference_location}}` in custom templates. /// - If the message contains `{{reference_location}}`, substitute it with the formatted location /// - Otherwise, prepend the location on its own line (default behavior) -fn format_violation_message(violation: &Violation, colorize: bool) -> String { +fn format_violation_message( + violation: &Violation, + color_mode: ColorMode, +) -> String { let location = format_location( &violation.identifier.file, violation.source_location.line, violation.source_location.column, - colorize, + color_mode, ); if violation.message.contains(REFERENCE_LOCATION_PLACEHOLDER) { @@ -50,7 +61,7 @@ fn format_violation_message(violation: &Violation, colorize: bool) -> String { pub fn write_text( result: &CheckAllResult, mut writer: W, - colorize: bool, + color_mode: ColorMode, ) -> anyhow::Result<()> { if !result.has_violations() { writeln!(writer, "No violations detected!")?; @@ -65,7 +76,7 @@ pub fn write_text( writeln!(writer, "{} violation(s) detected:", sorted_violations.len())?; for violation in sorted_violations { - let formatted = format_violation_message(violation, colorize); + let formatted = format_violation_message(violation, color_mode); writeln!(writer, "{}\n", formatted)?; } } @@ -115,20 +126,20 @@ mod tests { #[test] fn test_format_location_with_color() { - let result = format_location("foo.rb", 10, 5, true); + let result = format_location("foo.rb", 10, 5, ColorMode::Colored); assert_eq!(result, "\x1b[36mfoo.rb:10:5\x1b[0m"); } #[test] fn test_format_location_without_color() { - let result = format_location("foo.rb", 10, 5, false); + let result = format_location("foo.rb", 10, 5, ColorMode::Plain); assert_eq!(result, "foo.rb:10:5"); } #[test] fn test_format_violation_message_with_color() { let violation = sample_violation(); - let result = format_violation_message(&violation, true); + let result = format_violation_message(&violation, ColorMode::Colored); assert_eq!( result, "\x1b[36mfoo/bar/file.rb:10:5\x1b[0m\nPrivacy violation: `Foo` is private" @@ -138,7 +149,7 @@ mod tests { #[test] fn test_format_violation_message_without_color() { let violation = sample_violation(); - let result = format_violation_message(&violation, false); + let result = format_violation_message(&violation, ColorMode::Plain); assert_eq!( result, "foo/bar/file.rb:10:5\nPrivacy violation: `Foo` is private" @@ -154,7 +165,7 @@ mod tests { }; let mut output = Vec::new(); - write_text(&result, &mut output, false).unwrap(); + write_text(&result, &mut output, ColorMode::Plain).unwrap(); assert_eq!( String::from_utf8(output).unwrap(), "No violations detected!\n" @@ -170,7 +181,7 @@ mod tests { }; let mut output = Vec::new(); - write_text(&result, &mut output, false).unwrap(); + write_text(&result, &mut output, ColorMode::Plain).unwrap(); let text = String::from_utf8(output).unwrap(); assert!(text.contains("1 violation(s) detected:")); assert!(text.contains("foo/bar/file.rb:10:5")); @@ -200,7 +211,7 @@ mod tests { #[test] fn test_format_violation_message_with_custom_template_no_color() { let violation = custom_template_violation(); - let result = format_violation_message(&violation, false); + let result = format_violation_message(&violation, ColorMode::Plain); assert_eq!( result, "foo/bar/file.rb:10:5\nCustom privacy error for `Foo`" @@ -210,7 +221,7 @@ mod tests { #[test] fn test_format_violation_message_with_custom_template_with_color() { let violation = custom_template_violation(); - let result = format_violation_message(&violation, true); + let result = format_violation_message(&violation, ColorMode::Colored); assert_eq!( result, "\x1b[36mfoo/bar/file.rb:10:5\x1b[0m\nCustom privacy error for `Foo`" From 82d25f58f97406533599a60c2ccfe5e49fd44c46 Mon Sep 17 00:00:00 2001 From: Martin Emde Date: Wed, 7 Jan 2026 09:41:29 -0800 Subject: [PATCH 5/5] 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}} --- src/packs.rs | 6 +- src/packs/checker.rs | 134 ++-------------------- src/packs/checker/common_test.rs | 40 ++++++- src/packs/checker/dependency.rs | 20 ++-- src/packs/checker/folder_privacy.rs | 20 ++-- src/packs/checker/layer.rs | 32 +++--- src/packs/checker/pack_checker.rs | 123 ++++++++------------- src/packs/checker/privacy.rs | 22 ++-- src/packs/checker/visibility.rs | 20 ++-- src/packs/checker_configuration.rs | 44 +++++++- src/packs/csv.rs | 36 +++--- src/packs/json.rs | 55 +++++---- src/packs/pack.rs | 16 ++- src/packs/package_todo.rs | 2 +- src/packs/template.rs | 124 +++++++++++++++++++++ src/packs/text.rs | 166 +++++++++------------------- 16 files changed, 441 insertions(+), 419 deletions(-) create mode 100644 src/packs/template.rs diff --git a/src/packs.rs b/src/packs.rs index 84bedcc..204a0c9 100644 --- a/src/packs.rs +++ b/src/packs.rs @@ -18,6 +18,7 @@ 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; @@ -101,15 +102,16 @@ pub fn check( OutputFormat::Packwerk => { 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())?; } } diff --git a/src/packs/checker.rs b/src/packs/checker.rs index ddd56fd..cfd694b 100644 --- a/src/packs/checker.rs +++ b/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, + pub referencing_layer: Option, } pub(crate) trait CheckerInterface { @@ -82,77 +81,12 @@ pub struct CheckAllResult { pub strict_mode_violations: HashSet, } -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, @@ -543,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 diff --git a/src/packs/checker/common_test.rs b/src/packs/checker/common_test.rs index 3a1d69d..af53195 100644 --- a/src/packs/checker/common_test.rs +++ b/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() } } diff --git a/src/packs/checker/dependency.rs b/src/packs/checker/dependency.rs index 2d76090..2026a06 100644 --- a/src/packs/checker/dependency.rs +++ b/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( - "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 { @@ -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( - "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 { diff --git a/src/packs/checker/folder_privacy.rs b/src/packs/checker/folder_privacy.rs index a5a9edf..801d953 100644 --- a/src/packs/checker/folder_privacy.rs +++ b/src/packs/checker/folder_privacy.rs @@ -85,12 +85,14 @@ mod tests { enforce_folder_privacy: Some(CheckerSetting::True), ..default_defining_pack() }), - referencing_pack: Pack{ + referencing_pack: Pack { relative_path: PathBuf::from("packs/foo"), - ..default_referencing_pack()}, + ..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 { @@ -150,12 +152,14 @@ mod tests { enforce_folder_privacy: Some(CheckerSetting::Strict), ..default_defining_pack() }), - referencing_pack: Pack{ + referencing_pack: Pack { relative_path: PathBuf::from("packs/foo"), - ..default_referencing_pack()}, + ..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 { diff --git a/src/packs/checker/layer.rs b/src/packs/checker/layer.rs index bdc1e30..2805657 100644 --- a/src/packs/checker/layer.rs +++ b/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,8 @@ 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 +137,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; @@ -195,7 +191,7 @@ mod tests { referenced_constant_name: Some(String::from("::Bar")), defining_pack: Some(Pack { name: "packs/bar".to_owned(), - layer: Some("product".to_string()), + layer: Some("product".to_string()), ..default_defining_pack() }), referencing_pack: Pack { @@ -204,9 +200,12 @@ 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) } @@ -219,7 +218,7 @@ mod tests { referenced_constant_name: Some(String::from("::Bar")), defining_pack: Some(Pack { name: "packs/bar".to_owned(), - layer: Some("product".to_string()), + layer: Some("product".to_string()), ..default_defining_pack() }), referencing_pack: Pack { @@ -228,9 +227,12 @@ 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) } diff --git a/src/packs/checker/pack_checker.rs b/src/packs/checker/pack_checker.rs index 5cb732b..c99ef41 100644 --- a/src/packs/checker/pack_checker.rs +++ b/src/packs/checker/pack_checker.rs @@ -1,5 +1,3 @@ -use std::collections::HashMap; - use crate::packs::{ checker_configuration::{CheckerConfiguration, CheckerType}, pack::{CheckerSetting, Pack}, @@ -151,21 +149,34 @@ impl<'a> PackChecker<'a> { .is_ignored(file_path, &self.checker_configuration.checker_name()) } + /// Create a violation with optional layer information. + /// Template expansion is deferred to formatters. pub fn violation( &self, - extra_template_fields: Option>, + layer_info: Option<(&str, &str)>, ) -> anyhow::Result> { + let (defining_layer, referencing_layer) = match layer_info { + Some((def, ref_)) => { + (Some(def.to_string()), Some(ref_.to_string())) + } + None => (None, None), + }; Ok(Some(Violation { - message: self.interpolate_violation_message(extra_template_fields), identifier: self.violation_identifier(), source_location: self.reference.source_location.clone(), + referencing_pack_relative_yml: self + .referencing_pack + .relative_yml() + .to_string_lossy() + .to_string(), + defining_layer, + referencing_layer, })) } pub fn violation_identifier(&self) -> ViolationIdentifier { - let violation_type: &str = &self.checker_configuration.checker_name(); ViolationIdentifier { - violation_type: violation_type.to_string(), + violation_type: self.checker_type.clone(), strict: self.is_strict(), file: self.reference.relative_referencing_file.clone(), constant_name: self.reference.constant_name.clone(), @@ -173,49 +184,11 @@ impl<'a> PackChecker<'a> { defining_pack_name: self.defining_pack.unwrap().name.clone(), } } - - fn interpolate_violation_message( - &self, - extra_template_fields: Option>, - ) -> String { - let mut map = extra_template_fields.unwrap_or_default(); - map.insert( - "{{violation_name}}", - self.checker_configuration.pretty_checker_name(), - ); - map.insert( - "{{referencing_pack_name}}", - self.referencing_pack.name.clone(), - ); - map.insert( - "{{defining_pack_name}}", - self.defining_pack.unwrap().name.clone(), - ); - map.insert("{{constant_name}}", self.reference.constant_name.clone()); - map.insert( - "{{referencing_pack_relative_yml}}", - self.referencing_pack - .relative_yml() - .to_string_lossy() - .to_string(), - ); - // NOTE: {{reference_location}} is NOT substituted here. - // Formatters are responsible for substituting it with their preferred format - // (colorized for text output, etc.). This allows custom templates to use - // {{reference_location}} while giving formatters control over presentation. - - let mut interpolated_msg = - self.checker_configuration.checker_error_template(); - for (key, value) in &map { - interpolated_msg = interpolated_msg.replace(key, value); - } - interpolated_msg - } } #[cfg(test)] mod tests { - use std::collections::HashSet; + use std::collections::{HashMap, HashSet}; use crate::packs::{PackSet, SourceLocation}; @@ -302,26 +275,18 @@ mod tests { assert_eq!(checker.referencing_pack_name(), "packs/baz".to_string()); assert_eq!(checker.rules_checker_setting(), &CheckerSetting::False); assert!(!checker.violation_globally_disabled()); - let expected_violation_message: String = "Folder Privacy violation: `Foo` belongs to `packs/foo`, which is private to `packs/baz` as it is not a sibling pack or parent pack.".into(); - assert_eq!( - checker.interpolate_violation_message(None), - expected_violation_message - ); - let mut config = config; - let mut privacy_checker = - CheckerConfiguration::new(CheckerType::FolderPrivacy); - privacy_checker.override_error_template = - Some("{{violation_name}}".into()); - config - .checker_configuration - .insert(CheckerType::FolderPrivacy, privacy_checker); - let checker = - PackChecker::new(&config, CheckerType::FolderPrivacy, &refer)?; + // Test violation() creates correct data + let violation = checker.violation(None)?.unwrap(); assert_eq!( - checker.interpolate_violation_message(None), - "Folder Privacy".to_string() + violation.identifier.violation_type, + CheckerType::FolderPrivacy ); + assert_eq!(violation.identifier.constant_name, "Foo"); + assert_eq!(violation.identifier.defining_pack_name, "packs/foo"); + assert_eq!(violation.identifier.referencing_pack_name, "packs/baz"); + assert!(violation.defining_layer.is_none()); + assert!(violation.referencing_layer.is_none()); Ok(()) } @@ -332,11 +297,9 @@ mod tests { let checker = PackChecker::new(&config, CheckerType::Privacy, &refer)?; assert_eq!(checker.violation_direction(), ViolationDirection::Incoming); - let expected_violation_message: String = "Privacy violation: `Foo` is private to `packs/foo`, but referenced from `packs/baz`".into(); - assert_eq!( - checker.interpolate_violation_message(None), - expected_violation_message - ); + + let violation = checker.violation(None)?.unwrap(); + assert_eq!(violation.identifier.violation_type, CheckerType::Privacy); Ok(()) } @@ -348,10 +311,11 @@ mod tests { PackChecker::new(&config, CheckerType::Dependency, &refer)?; assert_eq!(checker.violation_direction(), ViolationDirection::Outgoing); - let expected_violation_message: String = "Dependency violation: `Foo` belongs to `packs/foo`, but `package.yml` does not specify a dependency on `packs/foo`.".into(); + + let violation = checker.violation(None)?.unwrap(); assert_eq!( - checker.interpolate_violation_message(None), - expected_violation_message + violation.identifier.violation_type, + CheckerType::Dependency ); Ok(()) @@ -363,11 +327,13 @@ mod tests { let checker = PackChecker::new(&config, CheckerType::Layer, &refer)?; assert_eq!(checker.violation_direction(), ViolationDirection::Outgoing); - let expected_violation_message: String = "Layer violation: `Foo` belongs to `packs/foo` (whose layer is `{{defining_layer}}`) cannot be accessed from `packs/baz` (whose layer is `{{referencing_layer}}`)".into(); - assert_eq!( - checker.interpolate_violation_message(None), - expected_violation_message - ); + + // Layer violations include layer info + let violation = + checker.violation(Some(("product", "utilities")))?.unwrap(); + assert_eq!(violation.identifier.violation_type, CheckerType::Layer); + assert_eq!(violation.defining_layer, Some("product".to_string())); + assert_eq!(violation.referencing_layer, Some("utilities".to_string())); Ok(()) } @@ -379,10 +345,11 @@ mod tests { PackChecker::new(&config, CheckerType::Visibility, &refer)?; assert_eq!(checker.violation_direction(), ViolationDirection::Incoming); - let expected_violation_message: String = "Visibility violation: `Foo` belongs to `packs/foo`, which is not visible to `packs/baz`".into(); + + let violation = checker.violation(None)?.unwrap(); assert_eq!( - checker.interpolate_violation_message(None), - expected_violation_message + violation.identifier.violation_type, + CheckerType::Visibility ); Ok(()) diff --git a/src/packs/checker/privacy.rs b/src/packs/checker/privacy.rs index 56b5186..b749f1b 100644 --- a/src/packs/checker/privacy.rs +++ b/src/packs/checker/privacy.rs @@ -166,13 +166,15 @@ mod tests { defining_pack: Some(Pack { name: "packs/bar".to_owned(), enforce_privacy: Some(CheckerSetting::True), - ignored_private_constants: HashSet::from([String::from("::Taco")]), + ignored_private_constants: HashSet::from([String::from( + "::Taco", + )]), ..default_defining_pack() }), referencing_pack: default_referencing_pack(), expected_violation: Some(build_expected_violation( - String::from("Privacy violation: `::Bar` is private to `packs/bar`, but referenced from `packs/foo`"), - String::from("privacy"), false, + CheckerType::Privacy, + false, )), }; test_check( @@ -194,13 +196,15 @@ mod tests { defining_pack: Some(Pack { name: "packs/bar".to_owned(), enforce_privacy: Some(CheckerSetting::Strict), - ignored_private_constants: HashSet::from([String::from("::Taco")]), + ignored_private_constants: HashSet::from([String::from( + "::Taco", + )]), ..default_defining_pack() }), referencing_pack: default_referencing_pack(), expected_violation: Some(build_expected_violation( - String::from("Privacy violation: `::Bar` is private to `packs/bar`, but referenced from `packs/foo`"), - String::from("privacy"), true, + CheckerType::Privacy, + true, )), }; test_check( @@ -453,9 +457,9 @@ mod tests { }), referencing_pack: default_referencing_pack(), expected_violation: Some(build_expected_violation_with_constant( - String::from("Privacy violation: `::Bar::BarChild` is private to `packs/bar`, but referenced from `packs/foo`"), - String::from("privacy"), false, - String::from("::Bar::BarChild") + CheckerType::Privacy, + false, + String::from("::Bar::BarChild"), )), ..Default::default() }; diff --git a/src/packs/checker/visibility.rs b/src/packs/checker/visibility.rs index a0be4dc..507e238 100644 --- a/src/packs/checker/visibility.rs +++ b/src/packs/checker/visibility.rs @@ -103,12 +103,14 @@ mod tests { enforce_visibility: Some(CheckerSetting::True), ..default_defining_pack() }), - referencing_pack: Pack{ + referencing_pack: Pack { relative_path: PathBuf::from("packs/foo"), - ..default_referencing_pack()}, + ..default_referencing_pack() + }, expected_violation: Some(build_expected_violation( - "Visibility violation: `::Bar` belongs to `packs/bar`, which is not visible to `packs/foo`".to_string(), - "visibility".to_string(), false)), + CheckerType::Visibility, + false, + )), }; test_check( &Checker { @@ -169,12 +171,14 @@ mod tests { enforce_visibility: Some(CheckerSetting::Strict), ..default_defining_pack() }), - referencing_pack: Pack{ + referencing_pack: Pack { relative_path: PathBuf::from("packs/foo"), - ..default_referencing_pack()}, + ..default_referencing_pack() + }, expected_violation: Some(build_expected_violation( - "Visibility violation: `::Bar` belongs to `packs/bar`, which is not visible to `packs/foo`".to_string(), - "visibility".to_string(), true)), + CheckerType::Visibility, + true, + )), }; test_check( &Checker { diff --git a/src/packs/checker_configuration.rs b/src/packs/checker_configuration.rs index 2117d37..53f0e27 100644 --- a/src/packs/checker_configuration.rs +++ b/src/packs/checker_configuration.rs @@ -1,4 +1,9 @@ -#[derive(Debug, PartialEq, Eq, Hash, Clone)] +use serde::Serialize; +use std::fmt; +use std::str::FromStr; + +#[derive(Debug, PartialEq, Eq, Hash, Clone, Serialize)] +#[serde(rename_all = "snake_case")] pub enum CheckerType { Dependency, Privacy, @@ -7,17 +12,44 @@ pub enum CheckerType { Visibility, } +impl fmt::Display for CheckerType { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + CheckerType::Dependency => write!(f, "dependency"), + CheckerType::Privacy => write!(f, "privacy"), + CheckerType::FolderPrivacy => write!(f, "folder_privacy"), + CheckerType::Layer => write!(f, "layer"), + CheckerType::Visibility => write!(f, "visibility"), + } + } +} + +impl FromStr for CheckerType { + type Err = String; + + fn from_str(s: &str) -> Result { + match s { + "dependency" => Ok(CheckerType::Dependency), + "privacy" => Ok(CheckerType::Privacy), + "folder_privacy" => Ok(CheckerType::FolderPrivacy), + "layer" => Ok(CheckerType::Layer), + "visibility" => Ok(CheckerType::Visibility), + _ => Err(format!("Unknown checker type: {}", s)), + } + } +} + #[derive(Debug, PartialEq, Clone)] pub struct CheckerConfiguration { pub checker_type: CheckerType, pub override_error_template: Option, } -const DEFAULT_DEPENDENCY_TEMPLATE: &str = "Dependency violation: `{{constant_name}}` belongs to `{{defining_pack_name}}`, but `{{referencing_pack_relative_yml}}` does not specify a dependency on `{{defining_pack_name}}`."; -const DEFAULT_FOLDER_PRIVACY_TEMPLATE: &str = "{{violation_name}} violation: `{{constant_name}}` belongs to `{{defining_pack_name}}`, which is private to `{{referencing_pack_name}}` as it is not a sibling pack or parent pack."; -const DEFAULT_LAYER_TEMPLATE: &str = "Layer violation: `{{constant_name}}` belongs to `{{defining_pack_name}}` (whose layer is `{{defining_layer}}`) cannot be accessed from `{{referencing_pack_name}}` (whose layer is `{{referencing_layer}}`)"; -const DEFAULT_VISIBILITY_TEMPLATE: &str = "Visibility violation: `{{constant_name}}` belongs to `{{defining_pack_name}}`, which is not visible to `{{referencing_pack_name}}`"; -const DEFAULT_PRIVACY_TEMPLATE: &str = "Privacy violation: `{{constant_name}}` is private to `{{defining_pack_name}}`, but referenced from `{{referencing_pack_name}}`"; +const DEFAULT_DEPENDENCY_TEMPLATE: &str = "{{reference_location}}Dependency violation: `{{constant_name}}` belongs to `{{defining_pack_name}}`, but `{{referencing_pack_relative_yml}}` does not specify a dependency on `{{defining_pack_name}}`."; +const DEFAULT_FOLDER_PRIVACY_TEMPLATE: &str = "{{reference_location}}{{violation_name}} violation: `{{constant_name}}` belongs to `{{defining_pack_name}}`, which is private to `{{referencing_pack_name}}` as it is not a sibling pack or parent pack."; +const DEFAULT_LAYER_TEMPLATE: &str = "{{reference_location}}Layer violation: `{{constant_name}}` belongs to `{{defining_pack_name}}` (whose layer is `{{defining_layer}}`) cannot be accessed from `{{referencing_pack_name}}` (whose layer is `{{referencing_layer}}`)"; +const DEFAULT_VISIBILITY_TEMPLATE: &str = "{{reference_location}}Visibility violation: `{{constant_name}}` belongs to `{{defining_pack_name}}`, which is not visible to `{{referencing_pack_name}}`"; +const DEFAULT_PRIVACY_TEMPLATE: &str = "{{reference_location}}Privacy violation: `{{constant_name}}` is private to `{{defining_pack_name}}`, but referenced from `{{referencing_pack_name}}`"; impl CheckerConfiguration { pub fn new(checker_type: CheckerType) -> Self { diff --git a/src/packs/csv.rs b/src/packs/csv.rs index 0c8dbb8..5193141 100644 --- a/src/packs/csv.rs +++ b/src/packs/csv.rs @@ -1,19 +1,29 @@ use itertools::chain; -use super::checker::{build_strict_violation_message, CheckAllResult}; +use super::checker::{ + build_strict_violation_message, CheckAllResult, Violation, +}; +use super::template::{build_violation_vars, expand}; +use super::Configuration; -fn format_message_with_location(v: &super::checker::Violation) -> String { - format!( - "{}:{}:{}\n{}", - v.identifier.file, - v.source_location.line, - v.source_location.column, - v.message - ) +/// Build message from violation using template expansion. +/// For CSV, reference_location uses the default plain format. +fn build_message(v: &Violation, config: &Configuration) -> String { + if v.identifier.strict { + build_strict_violation_message(&v.identifier) + } else { + let checker_config = + &config.checker_configuration[&v.identifier.violation_type]; + let template = checker_config.checker_error_template(); + // Uses default reference_location from build_violation_vars (plain format) + let vars = build_violation_vars(v, checker_config); + expand(&template, &vars) + } } pub fn write_csv( result: &CheckAllResult, + config: &Configuration, writer: W, ) -> anyhow::Result<()> { let mut wtr = csv::Writer::from_writer(writer); @@ -37,13 +47,9 @@ pub fn write_csv( for violation in all { let identifier = &violation.identifier; - let message = if violation.identifier.strict { - build_strict_violation_message(&violation.identifier) - } else { - format_message_with_location(violation) - }; + let message = build_message(violation, config); wtr.serialize(( - &identifier.violation_type, + identifier.violation_type.to_string(), &identifier.strict, &identifier.file, &identifier.constant_name, diff --git a/src/packs/json.rs b/src/packs/json.rs index 9239746..228dfbc 100644 --- a/src/packs/json.rs +++ b/src/packs/json.rs @@ -6,7 +6,12 @@ use itertools::chain; use serde::Serialize; -use super::checker::{build_strict_violation_message, CheckAllResult}; +use super::checker::{ + build_strict_violation_message, CheckAllResult, Violation, +}; +use super::checker_configuration::CheckerType; +use super::template::{build_violation_vars, expand}; +use super::Configuration; #[derive(Serialize)] struct JsonOutput<'a> { @@ -17,7 +22,7 @@ struct JsonOutput<'a> { #[derive(Serialize)] struct JsonViolation<'a> { - violation_type: &'a str, + violation_type: &'a CheckerType, file: &'a str, line: usize, column: usize, @@ -30,7 +35,7 @@ struct JsonViolation<'a> { #[derive(Serialize)] struct JsonStaleTodo<'a> { - violation_type: &'a str, + violation_type: &'a CheckerType, file: &'a str, constant_name: &'a str, referencing_pack_name: &'a str, @@ -45,8 +50,25 @@ struct JsonSummary { success: bool, } +/// Build message from violation using template expansion. +/// For JSON, reference_location is cleared (location is in separate fields). +fn build_message(v: &Violation, config: &Configuration) -> String { + if v.identifier.strict { + build_strict_violation_message(&v.identifier) + } else { + let checker_config = + &config.checker_configuration[&v.identifier.violation_type]; + let template = checker_config.checker_error_template(); + let mut vars = build_violation_vars(v, checker_config); + // Clear reference_location for JSON - location is in separate fields + vars.insert("reference_location", String::new()); + expand(&template, &vars) + } +} + pub fn write_json( result: &CheckAllResult, + config: &Configuration, writer: W, ) -> anyhow::Result<()> { let all_violations = chain!( @@ -57,23 +79,16 @@ pub fn write_json( // JSON outputs raw structured data - consumers can format as needed. // Location is provided as separate file/line/column fields. let violations: Vec = all_violations - .map(|v| { - let message = if v.identifier.strict { - build_strict_violation_message(&v.identifier) - } else { - v.message.clone() - }; - JsonViolation { - violation_type: &v.identifier.violation_type, - file: &v.identifier.file, - line: v.source_location.line, - column: v.source_location.column, - constant_name: &v.identifier.constant_name, - referencing_pack_name: &v.identifier.referencing_pack_name, - defining_pack_name: &v.identifier.defining_pack_name, - strict: v.identifier.strict, - message, - } + .map(|v| JsonViolation { + violation_type: &v.identifier.violation_type, + file: &v.identifier.file, + line: v.source_location.line, + column: v.source_location.column, + constant_name: &v.identifier.constant_name, + referencing_pack_name: &v.identifier.referencing_pack_name, + defining_pack_name: &v.identifier.defining_pack_name, + strict: v.identifier.strict, + message: build_message(v, config), }) .collect(); diff --git a/src/packs/pack.rs b/src/packs/pack.rs index abdf7e2..a207a43 100644 --- a/src/packs/pack.rs +++ b/src/packs/pack.rs @@ -175,11 +175,20 @@ impl CheckerSetting { impl Pack { pub fn all_violations(&self) -> Vec { + use super::checker_configuration::CheckerType; + use std::str::FromStr; + let mut violations = Vec::new(); let violations_by_pack = &self.package_todo.violations_by_defining_pack; for (defining_pack_name, violation_groups) in violations_by_pack { for (constant_name, violation_group) in violation_groups { - for violation_type in &violation_group.violation_types { + for violation_type_str in &violation_group.violation_types { + // Parse the string to CheckerType, skip unknown types + let Ok(violation_type) = + CheckerType::from_str(violation_type_str) + else { + continue; + }; for file in &violation_group.files { let identifier = ViolationIdentifier { violation_type: violation_type.clone(), @@ -780,9 +789,10 @@ ignored_private_constants: let mut actual = pack.all_violations(); actual.sort_by(|a, b| a.file.cmp(&b.file)); + use super::super::checker_configuration::CheckerType; let expected = vec![ ViolationIdentifier { - violation_type: "dependency".to_string(), + violation_type: CheckerType::Dependency, strict: false, file: "packs/foo/app/services/foo.rb".to_string(), constant_name: "::Bar".to_string(), @@ -790,7 +800,7 @@ ignored_private_constants: defining_pack_name: "packs/bar".to_string(), }, ViolationIdentifier { - violation_type: "dependency".to_string(), + violation_type: CheckerType::Dependency, strict: false, file: "packs/foo/app/services/other_foo.rb".to_string(), constant_name: "::Bar".to_string(), diff --git a/src/packs/package_todo.rs b/src/packs/package_todo.rs index 5c0f480..f1b638d 100644 --- a/src/packs/package_todo.rs +++ b/src/packs/package_todo.rs @@ -118,7 +118,7 @@ pub fn package_todos_for_pack_name( .insert(violation.identifier.file.to_owned()); violation_group .violation_types - .insert(violation.identifier.violation_type.to_owned()); + .insert(violation.identifier.violation_type.to_string()); } let package_todo = PackageTodo { diff --git a/src/packs/template.rs b/src/packs/template.rs new file mode 100644 index 0000000..fff715f --- /dev/null +++ b/src/packs/template.rs @@ -0,0 +1,124 @@ +//! Template expansion for violation messages. +//! +//! This module centralizes all template expansion logic. Formatters call +//! `build_violation_vars()` to get a variable map, then `expand()` to +//! substitute placeholders in templates. + +use std::collections::HashMap; + +use super::checker::Violation; +use super::checker_configuration::CheckerConfiguration; + +/// Expand a template by substituting all {{placeholder}} with values. +pub fn expand(template: &str, variables: &HashMap<&str, String>) -> String { + let mut result = template.to_string(); + for (key, value) in variables { + let placeholder = format!("{{{{{}}}}}", key); + result = result.replace(&placeholder, value); + } + result +} + +/// Format reference location as file:line:column with newline. +pub fn format_reference_location( + file: &str, + line: usize, + column: usize, +) -> String { + format!("{}:{}:{}\n", file, line, column) +} + +/// Build template variables from a Violation and its CheckerConfiguration. +/// Includes `reference_location` in plain format by default. +/// Callers can modify the returned map to colorize or clear it. +pub fn build_violation_vars( + v: &Violation, + checker_config: &CheckerConfiguration, +) -> HashMap<&'static str, String> { + let mut map = HashMap::new(); + map.insert("violation_name", checker_config.pretty_checker_name()); + map.insert("constant_name", v.identifier.constant_name.clone()); + map.insert( + "defining_pack_name", + v.identifier.defining_pack_name.clone(), + ); + map.insert( + "referencing_pack_name", + v.identifier.referencing_pack_name.clone(), + ); + map.insert( + "referencing_pack_relative_yml", + v.referencing_pack_relative_yml.clone(), + ); + // Include reference_location by default (plain format) + map.insert( + "reference_location", + format_reference_location( + &v.identifier.file, + v.source_location.line, + v.source_location.column, + ), + ); + // Layer-specific fields + if let Some(ref layer) = v.defining_layer { + map.insert("defining_layer", layer.clone()); + } + if let Some(ref layer) = v.referencing_layer { + map.insert("referencing_layer", layer.clone()); + } + map +} + +/// Wrap a reference location string with ANSI color codes. +pub fn colorize_reference_location(location: &str) -> String { + let trimmed = location.trim_end_matches('\n'); + let suffix = if location.ends_with('\n') { "\n" } else { "" }; + format!("\x1b[36m{}\x1b[0m{}", trimmed, suffix) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_expand_simple() { + let mut vars = HashMap::new(); + vars.insert("name", "World".to_string()); + assert_eq!(expand("Hello, {{name}}!", &vars), "Hello, World!"); + } + + #[test] + fn test_expand_multiple() { + let mut vars = HashMap::new(); + vars.insert("a", "1".to_string()); + vars.insert("b", "2".to_string()); + assert_eq!(expand("{{a}} + {{b}} = 3", &vars), "1 + 2 = 3"); + } + + #[test] + fn test_expand_missing_var() { + let vars = HashMap::new(); + assert_eq!(expand("Hello, {{name}}!", &vars), "Hello, {{name}}!"); + } + + #[test] + fn test_format_reference_location() { + assert_eq!(format_reference_location("foo.rb", 10, 5), "foo.rb:10:5\n"); + } + + #[test] + fn test_colorize_reference_location() { + assert_eq!( + colorize_reference_location("foo.rb:10:5\n"), + "\x1b[36mfoo.rb:10:5\x1b[0m\n" + ); + } + + #[test] + fn test_colorize_reference_location_no_newline() { + assert_eq!( + colorize_reference_location("foo.rb:10:5"), + "\x1b[36mfoo.rb:10:5\x1b[0m" + ); + } +} diff --git a/src/packs/text.rs b/src/packs/text.rs index cf27463..9736098 100644 --- a/src/packs/text.rs +++ b/src/packs/text.rs @@ -3,6 +3,13 @@ //! Formats check results as human-readable text with optional color output. use super::bin_locater; +use super::checker::{ + build_strict_violation_message, CheckAllResult, Violation, +}; +use super::template::{ + build_violation_vars, colorize_reference_location, expand, +}; +use super::Configuration; /// Controls whether output should include ANSI color codes. #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -10,56 +17,33 @@ pub enum ColorMode { Colored, Plain, } -use super::checker::{ - build_strict_violation_message, CheckAllResult, Violation, -}; - -/// Format a file:line:column location, optionally with color -fn format_location( - file: &str, - line: usize, - column: usize, - color_mode: ColorMode, -) -> String { - match color_mode { - ColorMode::Colored => { - format!("\x1b[36m{}:{}:{}\x1b[0m", file, line, column) - } - ColorMode::Plain => format!("{}:{}:{}", file, line, column), - } -} - -const REFERENCE_LOCATION_PLACEHOLDER: &str = "{{reference_location}}"; /// Format a violation message with optional colorization of the location. -/// -/// This function is responsible for substituting `{{reference_location}}` in custom templates. -/// - If the message contains `{{reference_location}}`, substitute it with the formatted location -/// - Otherwise, prepend the location on its own line (default behavior) fn format_violation_message( violation: &Violation, + config: &Configuration, color_mode: ColorMode, ) -> String { - let location = format_location( - &violation.identifier.file, - violation.source_location.line, - violation.source_location.column, - color_mode, - ); - - 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) + let checker_config = + &config.checker_configuration[&violation.identifier.violation_type]; + let template = checker_config.checker_error_template(); + + let mut vars = build_violation_vars(violation, checker_config); + // Colorize reference_location if color mode is enabled + if color_mode == ColorMode::Colored { + if let Some(loc) = vars.get("reference_location").cloned() { + vars.insert( + "reference_location", + colorize_reference_location(&loc), + ); + } } + expand(&template, &vars) } pub fn write_text( result: &CheckAllResult, + config: &Configuration, mut writer: W, color_mode: ColorMode, ) -> anyhow::Result<()> { @@ -71,12 +55,16 @@ pub fn write_text( if !result.reportable_violations.is_empty() { let mut sorted_violations: Vec<&Violation> = result.reportable_violations.iter().collect(); - sorted_violations.sort_by(|a, b| a.message.cmp(&b.message)); + sorted_violations.sort_by(|a, b| { + (&a.identifier.file, &a.identifier.constant_name) + .cmp(&(&b.identifier.file, &b.identifier.constant_name)) + }); writeln!(writer, "{} violation(s) detected:", sorted_violations.len())?; for violation in sorted_violations { - let formatted = format_violation_message(violation, color_mode); + let formatted = + format_violation_message(violation, config, color_mode); writeln!(writer, "{}\n", formatted)?; } } @@ -103,14 +91,14 @@ pub fn write_text( mod tests { use super::*; use crate::packs::checker::{Violation, ViolationIdentifier}; + use crate::packs::checker_configuration::CheckerType; use crate::packs::SourceLocation; use std::collections::HashSet; fn sample_violation() -> Violation { Violation { - message: "Privacy violation: `Foo` is private".to_string(), identifier: ViolationIdentifier { - violation_type: "Privacy".to_string(), + violation_type: CheckerType::Privacy, strict: false, file: "foo/bar/file.rb".to_string(), constant_name: "Foo".to_string(), @@ -121,43 +109,15 @@ mod tests { line: 10, column: 5, }, + referencing_pack_relative_yml: "bar/package.yml".to_string(), + defining_layer: None, + referencing_layer: None, } } - #[test] - fn test_format_location_with_color() { - let result = format_location("foo.rb", 10, 5, ColorMode::Colored); - assert_eq!(result, "\x1b[36mfoo.rb:10:5\x1b[0m"); - } - - #[test] - fn test_format_location_without_color() { - let result = format_location("foo.rb", 10, 5, ColorMode::Plain); - assert_eq!(result, "foo.rb:10:5"); - } - - #[test] - fn test_format_violation_message_with_color() { - let violation = sample_violation(); - let result = format_violation_message(&violation, ColorMode::Colored); - assert_eq!( - result, - "\x1b[36mfoo/bar/file.rb:10:5\x1b[0m\nPrivacy violation: `Foo` is private" - ); - } - - #[test] - fn test_format_violation_message_without_color() { - let violation = sample_violation(); - let result = format_violation_message(&violation, ColorMode::Plain); - assert_eq!( - result, - "foo/bar/file.rb:10:5\nPrivacy violation: `Foo` is private" - ); - } - #[test] fn test_write_text_no_violations() { + let config = Configuration::default(); let result = CheckAllResult { reportable_violations: HashSet::new(), stale_violations: Vec::new(), @@ -165,7 +125,7 @@ mod tests { }; let mut output = Vec::new(); - write_text(&result, &mut output, ColorMode::Plain).unwrap(); + write_text(&result, &config, &mut output, ColorMode::Plain).unwrap(); assert_eq!( String::from_utf8(output).unwrap(), "No violations detected!\n" @@ -174,6 +134,7 @@ mod tests { #[test] fn test_write_text_with_violations() { + let config = Configuration::default(); let result = CheckAllResult { reportable_violations: [sample_violation()].into_iter().collect(), stale_violations: Vec::new(), @@ -181,50 +142,27 @@ mod tests { }; let mut output = Vec::new(); - write_text(&result, &mut output, ColorMode::Plain).unwrap(); + write_text(&result, &config, &mut output, ColorMode::Plain).unwrap(); let text = String::from_utf8(output).unwrap(); assert!(text.contains("1 violation(s) detected:")); assert!(text.contains("foo/bar/file.rb:10:5")); - assert!(text.contains("Privacy violation: `Foo` is private")); - } - - fn custom_template_violation() -> Violation { - Violation { - // Message with {{reference_location}} placeholder (from custom template) - message: "{{reference_location}}Custom privacy error for `Foo`" - .to_string(), - identifier: ViolationIdentifier { - violation_type: "Privacy".to_string(), - strict: false, - file: "foo/bar/file.rb".to_string(), - constant_name: "Foo".to_string(), - referencing_pack_name: "bar".to_string(), - defining_pack_name: "foo".to_string(), - }, - source_location: SourceLocation { - line: 10, - column: 5, - }, - } + assert!(text.contains("Privacy violation")); + assert!(text.contains("`Foo`")); } #[test] - fn test_format_violation_message_with_custom_template_no_color() { - let violation = custom_template_violation(); - let result = format_violation_message(&violation, ColorMode::Plain); - assert_eq!( - result, - "foo/bar/file.rb:10:5\nCustom privacy error for `Foo`" - ); - } + fn test_write_text_with_color() { + let config = Configuration::default(); + let result = CheckAllResult { + reportable_violations: [sample_violation()].into_iter().collect(), + stale_violations: Vec::new(), + strict_mode_violations: HashSet::new(), + }; - #[test] - fn test_format_violation_message_with_custom_template_with_color() { - let violation = custom_template_violation(); - let result = format_violation_message(&violation, ColorMode::Colored); - assert_eq!( - result, - "\x1b[36mfoo/bar/file.rb:10:5\x1b[0m\nCustom privacy error for `Foo`" - ); + let mut output = Vec::new(); + write_text(&result, &config, &mut output, ColorMode::Colored).unwrap(); + let text = String::from_utf8(output).unwrap(); + // Check that ANSI codes are present for the location + assert!(text.contains("\x1b[36mfoo/bar/file.rb:10:5\x1b[0m")); } }