diff --git a/src/packs/checker.rs b/src/packs/checker.rs index cfd694b..9604219 100644 --- a/src/packs/checker.rs +++ b/src/packs/checker.rs @@ -39,6 +39,20 @@ pub struct ViolationIdentifier { pub referencing_pack_name: String, pub defining_pack_name: String, } + +impl ViolationIdentifier { + /// `strict` describes how a violation should be treated, not which violation + /// it is, and `package_todo.yml` has nowhere to record it, so recorded + /// violations are always rebuilt with `strict: false`. Compare through this + /// so a violation in a strict pack can still match its recorded entry. + pub fn recorded_key(&self) -> Self { + Self { + strict: false, + ..self.clone() + } + } +} + /// A violation combines an identifier with display metadata. /// /// `source_location` is intentionally separate from `ViolationIdentifier` because: @@ -142,7 +156,10 @@ impl<'a> CheckAllBuilder<'a> { self.found_violations .violations .iter() - .filter(|v| !recorded_violations.contains(&v.identifier)) + .filter(|v| { + !recorded_violations + .contains(&v.identifier.recorded_key()) + }) .collect() }; reportable_violations @@ -152,11 +169,11 @@ impl<'a> CheckAllBuilder<'a> { &mut self, recorded_violations: &'a HashSet, ) -> anyhow::Result> { - let found_violation_identifiers: HashSet<&ViolationIdentifier> = self + let found_violation_identifiers: HashSet = self .found_violations .violations .par_iter() - .map(|v| &v.identifier) + .map(|v| v.identifier.recorded_key()) .collect(); let relative_files = self .found_violations @@ -196,9 +213,13 @@ impl<'a> CheckAllBuilder<'a> { Ok(stale_violations) } + /// `found_violation_identifiers` is keyed by [`ViolationIdentifier::recorded_key`]. + /// `todo_violation_identifier` needs no such normalization: it comes from + /// `pack_set.all_violations`, which rebuilds every recorded violation with + /// `strict: false` already, so it is its own recorded key. fn is_stale_violation( relative_files: &HashSet<&str>, - found_violation_identifiers: &HashSet<&ViolationIdentifier>, + found_violation_identifiers: &HashSet, todo_violation_identifier: &ViolationIdentifier, ) -> bool { let violation_path_exists = diff --git a/tests/check_test.rs b/tests/check_test.rs index 4f0d043..85f4771 100644 --- a/tests/check_test.rs +++ b/tests/check_test.rs @@ -321,6 +321,10 @@ fn test_check_without_stale_violations() -> Result<(), Box> { #[test] fn test_check_with_strict_mode() -> Result<(), Box> { + // The violation here IS recorded in packs/foo/package_todo.yml, so it has to + // match its recorded entry: reported neither as a new violation nor as a + // stale todo. Strict mode still fails the run, which is what keeps this at + // exit 1, so the two strict messages are the whole of the output. cargo_bin_cmd!("pks") .arg("--project-root") .arg("tests/fixtures/uses_strict_mode") @@ -332,7 +336,14 @@ fn test_check_with_strict_mode() -> Result<(), Box> { )) .stdout(predicate::str::contains( "packs/foo cannot have dependency violations on packs/bar because strict mode is enabled for dependency violations in the enforcing pack's package.yml file", - )); + )) + .stdout( + predicate::str::contains( + "There were stale violations found, please run `packs update`", + ) + .not(), + ) + .stdout(predicate::str::contains("violation(s) detected:").not()); common::teardown(); Ok(())