diff --git a/src/packs.rs b/src/packs.rs index 3cc16a9..204a0c9 100644 --- a/src/packs.rs +++ b/src/packs.rs @@ -18,6 +18,8 @@ pub(crate) mod monkey_patch_detection; pub(crate) mod pack; pub(crate) mod parsing; pub(crate) mod raw_configuration; +pub(crate) mod template; +pub(crate) mod text; pub(crate) mod walk_directory; mod constant_dependencies; @@ -40,6 +42,7 @@ pub(crate) use self::parsing::ruby::zeitwerk::get_zeitwerk_constant_resolver; pub(crate) use self::parsing::ParsedDefinition; pub(crate) use self::parsing::UnresolvedReference; use anyhow::bail; +use cli::ColorChoice; use cli::OutputFormat; use cli::ViolationsFound; pub(crate) use configuration::Configuration; @@ -49,6 +52,7 @@ pub(crate) use package_todo::PackageTodo; use anyhow::Context; use serde::Deserialize; use serde::Serialize; +use std::io::IsTerminal; use std::path::PathBuf; pub fn greet() { @@ -70,9 +74,25 @@ pub fn create( Ok(()) } +/// Determine whether to use colors based on the color choice +fn color_mode_for(color: ColorChoice) -> text::ColorMode { + match color { + ColorChoice::Always => text::ColorMode::Colored, + ColorChoice::Never => text::ColorMode::Plain, + ColorChoice::Auto => { + if std::io::stdout().is_terminal() { + text::ColorMode::Colored + } else { + text::ColorMode::Plain + } + } + } +} + pub fn check( configuration: &Configuration, output_format: OutputFormat, + color: ColorChoice, files: Vec, ) -> anyhow::Result<()> { let result = checker::check_all(configuration, files) @@ -80,13 +100,18 @@ pub fn check( match output_format { OutputFormat::Packwerk => { - println!("{}", result); + text::write_text( + &result, + configuration, + std::io::stdout(), + color_mode_for(color), + )?; } OutputFormat::CSV => { - csv::write_csv(&result, std::io::stdout())?; + csv::write_csv(&result, configuration, std::io::stdout())?; } OutputFormat::JSON => { - json::write_json(&result, std::io::stdout())?; + json::write_json(&result, configuration, std::io::stdout())?; } } diff --git a/src/packs/checker.rs b/src/packs/checker.rs index 58544af..cfd694b 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; @@ -25,9 +24,6 @@ use rayon::prelude::IntoParallelRefIterator; use rayon::prelude::ParallelIterator; use reference::Reference; use std::collections::HashMap; -use std::fmt; -use std::fmt::Display; -use std::fmt::Formatter; use std::{collections::HashSet, path::PathBuf}; use tracing::debug; @@ -36,7 +32,7 @@ use super::reference_extractor::get_all_references; #[derive(PartialEq, Clone, Eq, Hash, Debug)] pub struct ViolationIdentifier { - pub violation_type: String, + pub violation_type: CheckerType, pub strict: bool, pub file: String, pub constant_name: String, @@ -52,11 +48,16 @@ pub struct ViolationIdentifier { /// one violation, even if they occur at different lines /// - Keeping line/column out of the identity makes violations stable across /// minor code movements that shift line numbers +/// +/// Violations store only data - template expansion happens in formatters. #[derive(PartialEq, Clone, Eq, Hash, Debug)] pub struct Violation { - pub message: String, pub identifier: ViolationIdentifier, pub source_location: SourceLocation, + // Additional data for template expansion: + pub referencing_pack_relative_yml: String, + pub defining_layer: Option, + pub referencing_layer: Option, } pub(crate) trait CheckerInterface { @@ -86,47 +87,6 @@ impl CheckAllResult { || !self.stale_violations.is_empty() || !self.strict_mode_violations.is_empty() } - - fn write_violations(&self, f: &mut Formatter<'_>) -> fmt::Result { - if !self.reportable_violations.is_empty() { - let mut sorted_violations: Vec<&Violation> = - self.reportable_violations.iter().collect(); - sorted_violations.sort_by(|a, b| a.message.cmp(&b.message)); - - writeln!(f, "{} violation(s) detected:", sorted_violations.len())?; - - for violation in sorted_violations { - writeln!(f, "{}\n", violation.message)?; - } - } - - if !self.stale_violations.is_empty() { - writeln!( - f, - "There were stale violations found, please run `{} update`", - bin_locater::packs_bin_name(), - )?; - } - - if !self.strict_mode_violations.is_empty() { - for v in self.strict_mode_violations.iter() { - let error_message = - build_strict_violation_message(&v.identifier); - writeln!(f, "{}", error_message)?; - } - } - Ok(()) - } -} - -impl Display for CheckAllResult { - fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { - if self.has_violations() { - self.write_violations(f) - } else { - write!(f, "No violations detected!") - } - } } struct CheckAllBuilder<'a> { configuration: &'a Configuration, @@ -517,57 +477,5 @@ fn remove_reference_to_dependency( write_pack_to_disk(&updated_pack)?; Ok(()) } -#[cfg(test)] -mod tests { - use std::collections::HashSet; - - use crate::packs::checker::{ - CheckAllResult, Violation, ViolationIdentifier, - }; - use crate::packs::SourceLocation; - - #[test] - fn test_write_violations() { - let chec_result = CheckAllResult { - reportable_violations: [Violation { - message: "foo/bar/file1.rb:10:5\nPrivacy violation: `::Foo::PrivateClass` is private to `foo`, but referenced from `bar`".to_string(), - identifier: ViolationIdentifier { - violation_type: "Privacy".to_string(), - strict: false, - file: "foo/bar/file1.rb".to_string(), - constant_name: "::Foo::PrivateClass".to_string(), - referencing_pack_name: "bar".to_string(), - defining_pack_name: "foo".to_string(), - }, - source_location: SourceLocation { line: 10, column: 5 }, - }, - Violation { - message: "foo/bar/file2.rb:15:3\nDependency violation: `::Foo::AnotherClass` is not allowed to depend on `::Bar::SomeClass`".to_string(), - identifier: ViolationIdentifier { - violation_type: "Dependency".to_string(), - strict: false, - file: "foo/bar/file2.rb".to_string(), - constant_name: "::Foo::AnotherClass".to_string(), - referencing_pack_name: "foo".to_string(), - defining_pack_name: "bar".to_string(), - }, - source_location: SourceLocation { line: 15, column: 3 }, - }].iter().cloned().collect(), - stale_violations: Vec::new(), - strict_mode_violations: HashSet::new(), - }; - - let expected_output = "2 violation(s) detected: -foo/bar/file1.rb:10:5 -Privacy violation: `::Foo::PrivateClass` is private to `foo`, but referenced from `bar` - -foo/bar/file2.rb:15:3 -Dependency violation: `::Foo::AnotherClass` is not allowed to depend on `::Bar::SomeClass` - -"; - - let actual = format!("{}", chec_result); - - assert_eq!(actual, expected_output); - } -} +// Note: Display impl was removed from CheckAllResult. Use write_text() directly with Configuration. +// Tests for text formatting are in text.rs diff --git a/src/packs/checker/common_test.rs b/src/packs/checker/common_test.rs index f090f70..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() } } @@ -138,21 +166,7 @@ pub mod tests { let result = checker.check(&reference, &configuration)?; - let stripped_result = match result { - Some(violation) => { - let stripped_message = - strip_ansi_escapes::strip(violation.message); - - Some(Violation { - message: String::from_utf8_lossy(&stripped_message) - .to_string(), - ..violation - }) - } - None => None, - }; - - assert_eq!(stripped_result, test_checker.expected_violation); + assert_eq!(result, test_checker.expected_violation); Ok(()) } diff --git a/src/packs/checker/dependency.rs b/src/packs/checker/dependency.rs index 095d110..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( - "packs/foo/app/services/foo.rb:3:1\nDependency violation: `::Bar` belongs to `packs/bar`, but `packs/foo/package.yml` does not specify a dependency on `packs/bar`.".to_string(), - "dependency".to_string(), false)), + CheckerType::Dependency, + false, + )), }; test_check( &Checker { @@ -248,13 +250,15 @@ mod tests { name: "packs/bar".to_owned(), ..default_defining_pack() }), - referencing_pack: Pack{ + referencing_pack: Pack { relative_path: PathBuf::from("packs/foo"), enforce_dependencies: Some(CheckerSetting::Strict), - ..default_referencing_pack()}, + ..default_referencing_pack() + }, expected_violation: Some(build_expected_violation( - "packs/foo/app/services/foo.rb:3:1\nDependency violation: `::Bar` belongs to `packs/bar`, but `packs/foo/package.yml` does not specify a dependency on `packs/bar`.".to_string(), - "dependency".to_string(), true)), + CheckerType::Dependency, + true, + )), }; test_check( &Checker { diff --git a/src/packs/checker/folder_privacy.rs b/src/packs/checker/folder_privacy.rs index b0e8749..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( - "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".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( - "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".to_string(), true)), + CheckerType::FolderPrivacy, + true, + )), }; test_check( &Checker { diff --git a/src/packs/checker/layer.rs b/src/packs/checker/layer.rs index a9d200f..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( - "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".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( - "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".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/output_helper.rs b/src/packs/checker/output_helper.rs deleted file mode 100644 index 8da5d15..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!( - "\x1b[36m{}\x1b[0m:{}:{}\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..c99ef41 100644 --- a/src/packs/checker/pack_checker.rs +++ b/src/packs/checker/pack_checker.rs @@ -1,15 +1,10 @@ -use std::collections::HashMap; - use crate::packs::{ checker_configuration::{CheckerConfiguration, CheckerType}, pack::{CheckerSetting, Pack}, 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, @@ -154,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(), @@ -176,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( - "{{reference_location}}", - print_reference_location(self.reference), - ); - map.insert( - "{{referencing_pack_relative_yml}}", - self.referencing_pack - .relative_yml() - .to_string_lossy() - .to_string(), - ); - - 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}; @@ -305,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 = "\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(); - 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(()) } @@ -335,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 = "\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(); - 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(()) } @@ -351,10 +311,11 @@ 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 violation = checker.violation(None)?.unwrap(); assert_eq!( - checker.interpolate_violation_message(None), - expected_violation_message + violation.identifier.violation_type, + CheckerType::Dependency ); Ok(()) @@ -366,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 = "\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(); - 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(()) } @@ -382,10 +345,11 @@ 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 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 8394421..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("packs/foo/app/services/foo.rb:3:1\nPrivacy 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("packs/foo/app/services/foo.rb:3:1\nPrivacy 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("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"), 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 2562050..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( - "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".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( - "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".to_string(), true)), + CheckerType::Visibility, + true, + )), }; test_check( &Checker { diff --git a/src/packs/checker_configuration.rs b/src/packs/checker_configuration.rs index cf11da4..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,6 +12,33 @@ 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, 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/src/packs/csv.rs b/src/packs/csv.rs index d1739b8..5193141 100644 --- a/src/packs/csv.rs +++ b/src/packs/csv.rs @@ -1,9 +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; + +/// 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); @@ -27,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 { - violation.message.to_string() - }; + 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 c9599a9..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!( @@ -54,24 +76,19 @@ 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 { - 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 new file mode 100644 index 0000000..9736098 --- /dev/null +++ b/src/packs/text.rs @@ -0,0 +1,168 @@ +//! 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, +}; +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)] +pub enum ColorMode { + Colored, + Plain, +} + +/// Format a violation message with optional colorization of the location. +fn format_violation_message( + violation: &Violation, + config: &Configuration, + color_mode: ColorMode, +) -> String { + 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<()> { + 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.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, config, color_mode); + 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::checker_configuration::CheckerType; + use crate::packs::SourceLocation; + use std::collections::HashSet; + + fn sample_violation() -> Violation { + Violation { + identifier: ViolationIdentifier { + violation_type: CheckerType::Privacy, + 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, + }, + referencing_pack_relative_yml: "bar/package.yml".to_string(), + defining_layer: None, + referencing_layer: None, + } + } + + #[test] + fn test_write_text_no_violations() { + let config = Configuration::default(); + let result = CheckAllResult { + reportable_violations: HashSet::new(), + stale_violations: Vec::new(), + strict_mode_violations: HashSet::new(), + }; + + let mut output = Vec::new(); + write_text(&result, &config, &mut output, ColorMode::Plain).unwrap(); + assert_eq!( + String::from_utf8(output).unwrap(), + "No violations detected!\n" + ); + } + + #[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(), + strict_mode_violations: HashSet::new(), + }; + + let mut output = Vec::new(); + 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")); + assert!(text.contains("`Foo`")); + } + + #[test] + 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(), + }; + + 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")); + } +} 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(()) +}