Fix stale check when deleted (#186)

* fixing check when todo file was deleted

* bumping version to 0.1.97

Cargo.toml

@@ -2,7 +2,7 @@
 
 [package]
 name = "pks"
-version = "0.1.96"
+version = "0.1.97"
 edition = "2021"
 description = "Welcome! Please see https://github.com/alexevanczuk/packs for more information!"
 license = "MIT"

src/packs/checker.rs

@@ -114,7 +114,7 @@ struct CheckAllBuilder<'a> {
     configuration: &'a Configuration,
     found_violations: &'a FoundViolations,
 }
-
+#[derive(Debug)]
 struct FoundViolations {
     absolute_paths: HashSet<PathBuf>,
     violations: HashSet<Violation>,
@@ -209,13 +209,30 @@ impl<'a> CheckAllBuilder<'a> {
         let stale_violations = recorded_violations
             .par_iter()
             .filter(|v_identifier| {
-                relative_files.contains(&v_identifier.file.as_str())
-                    && !found_violation_identifiers.contains(v_identifier)
+                Self::is_stale_violation(
+                    &relative_files,
+                    &found_violation_identifiers,
+                    v_identifier,
+                )
             })
             .collect::<Vec<&ViolationIdentifier>>();
         Ok(stale_violations)
     }
 
+    fn is_stale_violation(
+        relative_files: &HashSet<&str>,
+        found_violation_identifiers: &HashSet<&ViolationIdentifier>,
+        todo_violation_identifier: &ViolationIdentifier,
+    ) -> bool {
+        let violation_path_exists =
+            relative_files.contains(todo_violation_identifier.file.as_str());
+        if violation_path_exists {
+            !found_violation_identifiers.contains(todo_violation_identifier)
+        } else {
+            true // The todo violation references a file that no longer exists
+        }
+    }
+
     fn build_strict_mode_violations(&self) -> Vec<&'a ViolationIdentifier> {
         self.found_violations
             .violations
@@ -405,7 +422,6 @@ fn get_all_violations(
     checkers: &Vec<Box<dyn CheckerInterface + Send + Sync>>,
 ) -> anyhow::Result<HashSet<Violation>> {
     let references = get_all_references(configuration, absolute_paths)?;
-
     debug!("Running checkers on resolved references");
 
     let violations = checkers

tests/check_test.rs

@@ -131,14 +131,31 @@ fn test_check_with_stale_violations() -> Result<(), Box<dyn Error>> {
     Ok(())
 }
 
+#[test]
+fn test_check_with_stale_violations_when_file_no_longer_exists(
+) -> Result<(), Box<dyn Error>> {
+    Command::cargo_bin("packs")
+        .unwrap()
+        .arg("--project-root")
+        .arg("tests/fixtures/contains_stale_violations_no_file")
+        .arg("check")
+        .assert()
+        .failure()
+        .stdout(predicate::str::contains(
+            "There were stale violations found, please run `packs update`",
+        ));
+
+    common::teardown();
+    Ok(())
+}
+
 #[test]
 fn test_check_without_stale_violations() -> Result<(), Box<dyn Error>> {
     Command::cargo_bin("packs")
         .unwrap()
         .arg("--project-root")
         .arg("tests/fixtures/contains_package_todo")
         .arg("check")
-        .arg("packs/foo/app/services/foo.rb")
         .assert()
         .success()
         .stdout(

tests/fixtures/contains_stale_violations_no_file/packs/bar/app/services/bar.rb

@@ -0,0 +1,5 @@
+module Bar
+    def bar_you
+        Foo.foo_you
+    end
+end

tests/fixtures/contains_stale_violations_no_file/packs/bar/package.yml

@@ -0,0 +1 @@
+enforce_privacy: true

tests/fixtures/contains_stale_violations_no_file/packs/bar/package_todo.yml

@@ -0,0 +1,16 @@
+# This file contains a list of dependencies that are not part of the long term plan for the
+# 'packs/foo' package.
+# We should generally work to reduce this list over time.
+#
+# You can regenerate this file using the following command:
+#
+# bin/packwerk update-todo
+---
+packs/foo:
+  "::Foo":
+    violations:
+    - privacy
+    files:
+    - packs/bar/app/services/bar.rb
+    - packs/bar/app/services/file_was_deleted.rb
+

tests/fixtures/contains_stale_violations_no_file/packs/foo/app/services/foo.rb

@@ -0,0 +1,5 @@
+module Foo
+  def self.foo_you
+    'foo you'
+  end
+end

tests/fixtures/contains_stale_violations_no_file/packs/foo/package.yml

@@ -0,0 +1,2 @@
+enforce_privacy: true
+

tests/fixtures/contains_stale_violations_no_file/packwerk.yml

@@ -0,0 +1,23 @@
+# See: Setting up the configuration file
+# https://github.com/Shopify/packwerk/blob/main/USAGE.md#setting-up-the-configuration-file
+
+# List of patterns for folder paths to include
+# include:
+# - "**/*.{rb,rake,erb}"
+
+# List of patterns for folder paths to exclude
+# exclude:
+# - "{bin,node_modules,script,tmp,vendor}/**/*"
+
+# Patterns to find package configuration files
+# package_paths: "**/"
+
+# List of custom associations, if any
+# custom_associations:
+# - "cache_belongs_to"
+
+# Whether or not you want the cache enabled (disabled by default)
+cache: false
+
+# Where you want the cache to be stored (default below)
+# cache_directory: 'tmp/cache/packwerk'