Not writing strict violations to todos

src/packs/checker.rs

@@ -34,6 +34,7 @@ use super::reference_extractor::get_all_references;
 #[derive(PartialEq, Clone, Eq, Hash, Debug)]
 pub struct ViolationIdentifier {
     pub violation_type: String,
+    pub strict: bool,
     pub file: String,
     pub constant_name: String,
     pub referencing_pack_name: String,
@@ -119,12 +120,7 @@ impl CheckAllResult {
 
         if !self.strict_mode_violations.is_empty() {
             for v in self.strict_mode_violations.iter() {
-                let error_message = format!("{} cannot have {} violations on {} because strict mode is enabled for {} violations in the enforcing pack's package.yml file",
-                                        v.referencing_pack_name,
-                                        v.violation_type,
-                                        v.defining_pack_name,
-                                        v.violation_type
-            );
+                let error_message = build_strict_violation_message(v);
                 writeln!(f, "{}", error_message)?;
             }
         }
@@ -147,7 +143,6 @@ struct CheckAllBuilder<'a> {
 }
 
 struct FoundViolations {
-    checkers: Vec<Box<dyn CheckerInterface + Send + Sync>>,
     absolute_paths: HashSet<PathBuf>,
     violations: HashSet<Violation>,
 }
@@ -178,7 +173,7 @@ impl<'a> CheckAllBuilder<'a> {
                 .cloned()
                 .collect(),
             strict_mode_violations: self
-                .build_strict_mode_violations(recorded_violations)?
+                .build_strict_mode_violations()
                 .into_iter()
                 .cloned()
                 .collect(),
@@ -248,37 +243,13 @@ impl<'a> CheckAllBuilder<'a> {
         Ok(stale_violations)
     }
 
-    fn build_strict_mode_violations(
-        &mut self,
-        recorded_violations: &'a HashSet<ViolationIdentifier>,
-    ) -> anyhow::Result<Vec<&'a ViolationIdentifier>> {
-        let indexed_checkers: HashMap<
-            String,
-            &Box<dyn CheckerInterface + Send + Sync>,
-        > = self
-            .found_violations
-            .checkers
-            .iter()
-            .map(|checker| (checker.violation_type(), checker))
-            .collect();
-
-        recorded_violations
+    fn build_strict_mode_violations(&self) -> Vec<&'a ViolationIdentifier> {
+        self.found_violations
+            .violations
             .iter()
-            .try_fold(vec![], |mut acc, violation| {
-                let checker = indexed_checkers
-                    .get(&violation.violation_type)
-                    .context(format!(
-                    "Checker for violation type {} not found",
-                    violation.violation_type
-                ))?;
-
-                if checker
-                    .is_strict_mode_violation(violation, self.configuration)?
-                {
-                    acc.push(violation);
-                }
-                Ok(acc)
-            })
+            .filter(|v| v.identifier.strict)
+            .map(|v| &v.identifier)
+            .collect()
     }
 }
 
@@ -295,7 +266,6 @@ pub(crate) fn check_all(
     let violations: HashSet<Violation> =
         get_all_violations(configuration, &absolute_paths, &checkers)?;
     let found_violations = FoundViolations {
-        checkers,
         absolute_paths,
         violations,
     };
@@ -322,6 +292,16 @@ fn validate(configuration: &Configuration) -> Vec<String> {
     validation_errors
 }
 
+pub(crate) fn build_strict_violation_message(
+    violation_identifier: &ViolationIdentifier,
+) -> String {
+    format!("{} cannot have {} violations on {} because strict mode is enabled for {} violations in the enforcing pack's package.yml file",
+    violation_identifier.referencing_pack_name,
+    violation_identifier.violation_type,
+    violation_identifier.defining_pack_name,
+    violation_identifier.violation_type,)
+}
+
 pub(crate) fn validate_all(
     configuration: &Configuration,
 ) -> anyhow::Result<()> {
@@ -348,8 +328,24 @@ pub(crate) fn update(configuration: &Configuration) -> anyhow::Result<()> {
         &checkers,
     )?;
 
+    let strict_violations = &violations
+        .iter()
+        .filter(|v| v.identifier.strict)
+        .collect::<Vec<&Violation>>();
+    if !strict_violations.is_empty() {
+        for violation in strict_violations {
+            let strict_message =
+                build_strict_violation_message(&violation.identifier);
+            println!("{}", strict_message);
+        }
+        println!(
+            "{} strict mode violation(s) detected. These violations must be fixed for `check` to succeed.",
+            &strict_violations.len()
+        );
+    }
     package_todo::write_violations_to_disk(configuration, violations);
     println!("Successfully updated package_todo.yml files!");
+
     Ok(())
 }
 

src/packs/checker/architecture.rs

@@ -171,6 +171,7 @@ impl CheckerInterface for Checker {
                 let file = reference.relative_referencing_file.clone();
                 let identifier = ViolationIdentifier {
                     violation_type,
+                    strict: referencing_pack.enforce_architecture().is_strict(),
                     file,
                     constant_name: reference.constant_name.clone(),
                     referencing_pack_name: referencing_pack_name.clone(),
@@ -250,7 +251,7 @@ mod tests {
             },
             expected_violation: Some(build_expected_violation(
                 "packs/foo/app/services/foo.rb:3:1\nArchitecture violation: `::Bar` belongs to `packs/bar` (whose layer is `product`) cannot be accessed from `packs/foo` (whose layer is `utilities`)".to_string(), 
-                "architecture".to_string())),
+                "architecture".to_string(), false)),
             ..Default::default()
         };
         test_check(&checker_with_layers(), &mut test_checker)
@@ -275,7 +276,32 @@ mod tests {
             },
             expected_violation: Some(build_expected_violation(
                 "packs/foo/app/services/foo.rb:3:1\nArchitecture violation: `::Bar` belongs to `packs/bar` (whose layer is `product`) cannot be accessed from `packs/foo` (whose layer is `utilities`)".to_string(), 
-                "architecture".to_string())),
+                "architecture".to_string(), false)),
+            ..Default::default()
+        };
+        test_check(&checker_with_layers(), &mut test_checker)
+    }
+
+    #[test]
+    fn reference_is_a_strict_violation() -> anyhow::Result<()> {
+        let mut test_checker = TestChecker {
+            reference: None,
+            configuration: None,
+            referenced_constant_name: Some(String::from("::Bar")),
+            defining_pack: Some(Pack {
+                name: "packs/bar".to_owned(),
+               layer: Some("product".to_string()),
+                ..default_defining_pack()
+            }),
+            referencing_pack: Pack {
+                name: "packs/foo".to_owned(),
+                enforce_architecture: Some(CheckerSetting::Strict),
+                layer: Some("utilities".to_string()),
+                ..default_referencing_pack()
+            },
+            expected_violation: Some(build_expected_violation(
+                "packs/foo/app/services/foo.rb:3:1\nArchitecture violation: `::Bar` belongs to `packs/bar` (whose layer is `product`) cannot be accessed from `packs/foo` (whose layer is `utilities`)".to_string(), 
+                "architecture".to_string(), true)),
             ..Default::default()
         };
         test_check(&checker_with_layers(), &mut test_checker)

src/packs/checker/common_test.rs

@@ -28,25 +28,29 @@ pub mod tests {
     pub fn build_expected_violation(
         message: String,
         violation_type: String,
+        strict: bool,
     ) -> Violation {
         build_expected_violation_with_constant(
             message,
             violation_type,
+            strict,
             String::from("::Bar"),
         )
     }
 
     pub fn build_expected_violation_with_constant(
         message: String,
         violation_type: String,
+        strict: bool,
         constant_name: String,
     ) -> Violation {
         Violation {
             message,
             identifier: ViolationIdentifier {
                 violation_type,
+                strict,
                 file: String::from("packs/foo/app/services/foo.rb"),
-                constant_name: constant_name,
+                constant_name,
                 referencing_pack_name: String::from("packs/foo"),
                 defining_pack_name: String::from("packs/bar"),
             },

src/packs/checker/dependency.rs

@@ -165,6 +165,7 @@ impl CheckerInterface for Checker {
             let file = reference.relative_referencing_file.clone();
             let identifier = ViolationIdentifier {
                 violation_type,
+                strict: referencing_pack.enforce_dependencies().is_strict(),
                 file,
                 constant_name: reference.constant_name.clone(),
                 referencing_pack_name: referencing_pack_name.clone(),
@@ -248,7 +249,29 @@ mod tests {
                 ..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())),
+                "dependency".to_string(), false)),
+            ..Default::default()
+        };
+        test_check(&Checker {}, &mut test_checker)
+    }
+
+    #[test]
+    fn test_with_strict_violation() -> anyhow::Result<()> {
+        let mut test_checker = TestChecker {
+            reference: None,
+            configuration: None,
+            referenced_constant_name: Some(String::from("::Bar")),
+            defining_pack: Some(Pack {
+                name: "packs/bar".to_owned(),
+                ..default_defining_pack()
+            }),
+            referencing_pack: Pack{
+                relative_path: PathBuf::from("packs/foo"),
+                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".to_string(), true)),
             ..Default::default()
         };
         test_check(&Checker {}, &mut test_checker)

src/packs/checker/folder_visibility.rs

@@ -35,6 +35,7 @@ impl CheckerInterface for Checker {
             );
             let identifier = ViolationIdentifier {
                 violation_type: self.violation_type(),
+                strict: defining_pack.enforce_folder_visibility().is_strict(),
                 file: reference.relative_referencing_file.clone(),
                 constant_name: reference.constant_name.clone(),
                 referencing_pack_name: referencing_pack.name.clone(),
@@ -117,7 +118,28 @@ mod tests {
                 ..default_referencing_pack()},
             expected_violation: Some(build_expected_violation(
                 "packs/foo/app/services/foo.rb:3:1\nFolder Visibility violation: `::Bar` belongs to `packs/bar`, which is not visible to `packs/foo` as it is not a sibling pack or parent pack.".to_string(),
-                "folder_visibility".to_string())),
+                "folder_visibility".to_string(), false)),
+            ..Default::default()
+        };
+        test_check(&Checker {}, &mut test_checker)
+    }
+    #[test]
+    fn test_with_strict_violation() -> anyhow::Result<()> {
+        let mut test_checker = TestChecker {
+            reference: None,
+            configuration: None,
+            referenced_constant_name: Some(String::from("::Bar")),
+            defining_pack: Some(Pack {
+                name: "packs/bar".to_owned(),
+                enforce_folder_visibility: Some(CheckerSetting::Strict),
+                ..default_defining_pack()
+            }),
+            referencing_pack: Pack{
+                relative_path: PathBuf::from("packs/foo"),
+                ..default_referencing_pack()},
+            expected_violation: Some(build_expected_violation(
+                "packs/foo/app/services/foo.rb:3:1\nFolder Visibility violation: `::Bar` belongs to `packs/bar`, which is not visible to `packs/foo` as it is not a sibling pack or parent pack.".to_string(),
+                "folder_visibility".to_string(), true)),
             ..Default::default()
         };
         test_check(&Checker {}, &mut test_checker)

src/packs/checker/privacy.rs

@@ -102,6 +102,7 @@ impl CheckerInterface for Checker {
         let file = reference.relative_referencing_file.clone();
         let identifier = ViolationIdentifier {
             violation_type,
+            strict: defining_pack.enforce_privacy().is_strict(),
             file,
             constant_name: reference.constant_name.clone(),
             referencing_pack_name: referencing_pack_name.clone(),
@@ -201,7 +202,29 @@ 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"),
+                String::from("privacy"), false,
+            )),
+            ..Default::default()
+        };
+        test_check(&Checker {}, &mut test_checker)
+    }
+
+    #[test]
+    fn test_with_strict_privacy_violation() -> anyhow::Result<()> {
+        let mut test_checker = TestChecker {
+            reference: None,
+            configuration: None,
+            referenced_constant_name: Some(String::from("::Bar")),
+            defining_pack: Some(Pack {
+                name: "packs/bar".to_owned(),
+                enforce_privacy: Some(CheckerSetting::Strict),
+                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,
             )),
             ..Default::default()
         };
@@ -376,7 +399,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"),
+                String::from("privacy"), false,
                 String::from("::Bar::BarChild")
             )),
             ..Default::default()

src/packs/checker/visibility.rs

@@ -64,6 +64,7 @@ impl CheckerInterface for Checker {
         let file = reference.relative_referencing_file.clone();
         let identifier = ViolationIdentifier {
             violation_type,
+            strict: defining_pack.enforce_visibility().is_strict(),
             file,
             constant_name: reference.constant_name.clone(),
             referencing_pack_name: referencing_pack_name.clone(),
@@ -144,7 +145,29 @@ mod tests {
                 ..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())),
+                "visibility".to_string(), false)),
+            ..Default::default()
+        };
+        test_check(&Checker {}, &mut test_checker)
+    }
+
+    #[test]
+    fn test_with_strict_violation() -> anyhow::Result<()> {
+        let mut test_checker = TestChecker {
+            reference: None,
+            configuration: None,
+            referenced_constant_name: Some(String::from("::Bar")),
+            defining_pack: Some(Pack {
+                name: "packs/bar".to_owned(),
+                enforce_visibility: Some(CheckerSetting::Strict),
+                ..default_defining_pack()
+            }),
+            referencing_pack: Pack{
+                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".to_string(), true)),
             ..Default::default()
         };
         test_check(&Checker {}, &mut test_checker)