From e70bdd4da2894866ccfc6190c7887a72974e48cb Mon Sep 17 00:00:00 2001
From: Gannon McGibbon <gannon.mcgibbon@gmail.com>
Date: Mon, 13 Mar 2023 18:29:27 -0500
Subject: [PATCH 1/6] Type PackageTodo entries

---
 .../formatters/default_offenses_formatter.rb  |  6 +++---
 lib/packwerk/package_todo.rb                  | 19 ++++++++++---------
 2 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/lib/packwerk/formatters/default_offenses_formatter.rb b/lib/packwerk/formatters/default_offenses_formatter.rb
index d3ef229df..6f12abaa3 100644
--- a/lib/packwerk/formatters/default_offenses_formatter.rb
+++ b/lib/packwerk/formatters/default_offenses_formatter.rb
@@ -20,9 +20,9 @@ def show_offenses(offenses)
         EOS
       end
 
-      sig { override.params(offense_collection: OffenseCollection, fileset: T::Set[String]).returns(String) }
-      def show_stale_violations(offense_collection, fileset)
-        if offense_collection.stale_violations?(fileset)
+      sig { override.params(offense_collection: OffenseCollection, file_set: T::Set[String]).returns(String) }
+      def show_stale_violations(offense_collection, file_set)
+        if offense_collection.stale_violations?(file_set)
           "There were stale violations found, please run `packwerk update-todo`"
         else
           "No stale violations detected"
diff --git a/lib/packwerk/package_todo.rb b/lib/packwerk/package_todo.rb
index 25aec5594..312285e18 100644
--- a/lib/packwerk/package_todo.rb
+++ b/lib/packwerk/package_todo.rb
@@ -7,8 +7,9 @@ module Packwerk
   class PackageTodo
     extend T::Sig
 
+    EntryType = T.type_alias { T::Hash[String, T::Hash[String, T::Array[String]]] }
     EntriesType = T.type_alias do
-      T::Hash[String, T.untyped]
+      T::Hash[String, EntryType]
     end
 
     sig { params(package: Packwerk::Package, filepath: String).void }
@@ -41,10 +42,10 @@ def add_entries(reference, violation_type)
       entries_for_constant = package_violations[reference.constant.name] ||= {}
 
       entries_for_constant["violations"] ||= []
-      entries_for_constant["violations"] << violation_type
+      entries_for_constant.fetch("violations") << violation_type
 
       entries_for_constant["files"] ||= []
-      entries_for_constant["files"] << reference.relative_path.to_s
+      entries_for_constant.fetch("files") << reference.relative_path.to_s
 
       @new_entries[reference.constant.package.name] = package_violations
       listed?(reference, violation_type: violation_type)
@@ -55,9 +56,9 @@ def stale_violations?(for_files)
       prepare_entries_for_dump
 
       todo_list.any? do |package, package_violations|
-        package_violations_for_files = {}
+        package_violations_for_files = T.let({}, EntryType)
         package_violations.each do |constant_name, entries_for_constant|
-          entries_for_files = for_files & entries_for_constant["files"]
+          entries_for_files = for_files & entries_for_constant.fetch("files")
           next if entries_for_files.none?
 
           package_violations_for_files[constant_name] = {
@@ -78,9 +79,9 @@ def stale_violations?(for_files)
           # If this list is empty, we also must have stale violations.
           next true if new_entries_violation_types.nil?
 
-          if entries_for_constant["violations"].all? { |type| new_entries_violation_types.include?(type) }
+          if entries_for_constant.fetch("violations").all? { |type| new_entries_violation_types.include?(type) }
             stale_violations =
-              entries_for_constant["files"] - Array(@new_entries.dig(package, constant_name, "files"))
+              entries_for_constant.fetch("files") - Array(@new_entries.dig(package, constant_name, "files"))
             stale_violations.any?
           else
             return true
@@ -122,8 +123,8 @@ def delete_if_exists
     def prepare_entries_for_dump
       @new_entries.each do |package_name, package_violations|
         package_violations.each do |_, entries_for_constant|
-          entries_for_constant["violations"].sort!.uniq!
-          entries_for_constant["files"].sort!.uniq!
+          entries_for_constant.fetch("violations").sort!.uniq!
+          entries_for_constant.fetch("files").sort!.uniq!
         end
         @new_entries[package_name] = package_violations.sort.to_h
       end

From 31529e02f387f026b37c72a56c018f3ba8a7fe7b Mon Sep 17 00:00:00 2001
From: Gannon McGibbon <gannon.mcgibbon@gmail.com>
Date: Mon, 13 Mar 2023 18:42:59 -0500
Subject: [PATCH 2/6] Improve OffenseCollection#stale_violations? test accuracy

---
 test/unit/packwerk/offense_collection_test.rb | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/test/unit/packwerk/offense_collection_test.rb b/test/unit/packwerk/offense_collection_test.rb
index ae0dc2256..eac7dfa5e 100644
--- a/test/unit/packwerk/offense_collection_test.rb
+++ b/test/unit/packwerk/offense_collection_test.rb
@@ -27,19 +27,23 @@ class OffenseCollectionTest < Minitest::Test
 
     test "#stale_violations? returns true if there are stale violations" do
       @offense_collection.add_offense(@offense)
+      file_set = Set.new
 
       Packwerk::PackageTodo.any_instance
         .expects(:stale_violations?)
+        .with(file_set)
         .returns(true)
 
-      assert @offense_collection.stale_violations?(Set.new)
+      assert @offense_collection.stale_violations?(file_set)
     end
 
     test "#stale_violations? returns false if no stale violations" do
       @offense_collection.add_offense(@offense)
+      file_set = Set.new
 
       Packwerk::PackageTodo.any_instance
         .expects(:stale_violations?)
+        .with(file_set)
         .returns(false)
 
       refute @offense_collection.stale_violations?(Set.new)

From d3576d58f5bd2506abd584cf3dabf8c7d19246bc Mon Sep 17 00:00:00 2001
From: Gannon McGibbon <gannon.mcgibbon@gmail.com>
Date: Mon, 13 Mar 2023 19:36:35 -0500
Subject: [PATCH 3/6] Refactor PackageTodo#stale_violations?

Breaking it up into private methods for easier readability.
---
 lib/packwerk/package_todo.rb | 72 ++++++++++++++++++++++--------------
 1 file changed, 44 insertions(+), 28 deletions(-)

diff --git a/lib/packwerk/package_todo.rb b/lib/packwerk/package_todo.rb
index 312285e18..c3148800b 100644
--- a/lib/packwerk/package_todo.rb
+++ b/lib/packwerk/package_todo.rb
@@ -55,38 +55,15 @@ def add_entries(reference, violation_type)
     def stale_violations?(for_files)
       prepare_entries_for_dump
 
-      todo_list.any? do |package, package_violations|
-        package_violations_for_files = T.let({}, EntryType)
-        package_violations.each do |constant_name, entries_for_constant|
-          entries_for_files = for_files & entries_for_constant.fetch("files")
-          next if entries_for_files.none?
-
-          package_violations_for_files[constant_name] = {
-            "violations" => entries_for_constant["violations"],
-            "files" => entries_for_files.to_a,
-          }
-        end
+      todo_list.any? do |package, violations|
+        violations_for_files = package_violations_for(violations, files: for_files)
 
         # We `next false` because if we cannot find existing violations for `for_files` within
         # the `package_todo.yml` file, then there are no violations that
         # can be considered stale.
-        next false if package_violations_for_files.empty?
-
-        package_violations_for_files.any? do |constant_name, entries_for_constant|
-          new_entries_violation_types = @new_entries.dig(package, constant_name, "violations")
-          # If there are no NEW entries that match the old entries `for_files`,
-          # @new_entries is from the list of violations we get when we check this file.
-          # If this list is empty, we also must have stale violations.
-          next true if new_entries_violation_types.nil?
-
-          if entries_for_constant.fetch("violations").all? { |type| new_entries_violation_types.include?(type) }
-            stale_violations =
-              entries_for_constant.fetch("files") - Array(@new_entries.dig(package, constant_name, "files"))
-            stale_violations.any?
-          else
-            return true
-          end
-        end
+        next false if violations_for_files.empty?
+
+        stale_violation_for_package?(package, violations: violations_for_files)
       end
     end
 
@@ -119,6 +96,45 @@ def delete_if_exists
 
     private
 
+    sig { params(package: String, violations: EntryType).returns(T::Boolean) }
+    def stale_violation_for_package?(package, violations:)
+      violations.any? do |constant_name, entries_for_constant|
+        new_entries_violation_types = T.cast(
+          @new_entries.dig(package, constant_name, "violations"),
+          T.nilable(T::Array[String]),
+        )
+        # If there are no NEW entries that match the old entries `for_files`,
+        # @new_entries is from the list of violations we get when we check this file.
+        # If this list is empty, we also must have stale violations.
+        next true if new_entries_violation_types.nil?
+
+        if entries_for_constant.fetch("violations").all? { |type| new_entries_violation_types.include?(type) }
+          stale_violations =
+            entries_for_constant.fetch("files") - Array(@new_entries.dig(package, constant_name, "files"))
+          stale_violations.any?
+        else
+          return true
+        end
+      end
+    end
+
+    sig { params(package_violations: EntryType, files: T::Set[String]).returns(EntryType) }
+    def package_violations_for(package_violations, files:)
+      {}.tap do |package_violations_for_files|
+        package_violations_for_files = T.cast(package_violations_for_files, EntryType)
+
+        package_violations.each do |constant_name, entries_for_constant|
+          entries_for_files = files & entries_for_constant.fetch("files")
+          next if entries_for_files.none?
+
+          package_violations_for_files[constant_name] = {
+            "violations" => entries_for_constant["violations"],
+            "files" => entries_for_files.to_a,
+          }
+        end
+      end
+    end
+
     sig { returns(EntriesType) }
     def prepare_entries_for_dump
       @new_entries.each do |package_name, package_violations|

From ecf165b449bf1cbdf743abab1704e0ec2b24ae8e Mon Sep 17 00:00:00 2001
From: Gannon McGibbon <gannon.mcgibbon@gmail.com>
Date: Tue, 14 Mar 2023 00:40:34 -0500
Subject: [PATCH 4/6] Detect deleted files as stale violations

Also rename todo_list to old_entries (to contrast with new_entries),
drop Type postfix from TODO entry types, and remove extra file entry
from test/fixtures/package_todo.yml to make an existing stale check test
pass.
---
 lib/packwerk/package_todo.rb            | 43 ++++++++++++++++---------
 test/fixtures/package_todo.yml          |  1 -
 test/unit/packwerk/package_todo_test.rb |  9 ++++++
 test/unit/packwerk/parse_run_test.rb    |  6 ++--
 4 files changed, 39 insertions(+), 20 deletions(-)

diff --git a/lib/packwerk/package_todo.rb b/lib/packwerk/package_todo.rb
index c3148800b..5347a4e28 100644
--- a/lib/packwerk/package_todo.rb
+++ b/lib/packwerk/package_todo.rb
@@ -7,17 +7,20 @@ module Packwerk
   class PackageTodo
     extend T::Sig
 
-    EntryType = T.type_alias { T::Hash[String, T::Hash[String, T::Array[String]]] }
-    EntriesType = T.type_alias do
-      T::Hash[String, EntryType]
+    PackageName = T.type_alias { String }
+    ConstantName = T.type_alias { String }
+    FilePath = T.type_alias { String }
+    Entry = T.type_alias { T::Hash[ConstantName, T::Hash[ConstantName, T::Array[FilePath]]] }
+    Entries = T.type_alias do
+      T::Hash[PackageName, Entry]
     end
 
     sig { params(package: Packwerk::Package, filepath: String).void }
     def initialize(package, filepath)
       @package = package
       @filepath = filepath
-      @new_entries = T.let({}, EntriesType)
-      @todo_list = T.let(nil, T.nilable(EntriesType))
+      @new_entries = T.let({}, Entries)
+      @old_entries = T.let(nil, T.nilable(Entries))
     end
 
     sig do
@@ -25,7 +28,7 @@ def initialize(package, filepath)
         .returns(T::Boolean)
     end
     def listed?(reference, violation_type:)
-      violated_constants_found = todo_list.dig(reference.constant.package.name, reference.constant.name)
+      violated_constants_found = old_entries.dig(reference.constant.package.name, reference.constant.name)
       return false unless violated_constants_found
 
       violated_constant_in_file = violated_constants_found.fetch("files", []).include?(reference.relative_path)
@@ -55,8 +58,9 @@ def add_entries(reference, violation_type)
     def stale_violations?(for_files)
       prepare_entries_for_dump
 
-      todo_list.any? do |package, violations|
-        violations_for_files = package_violations_for(violations, files: for_files)
+      old_entries.any? do |package, violations|
+        files = for_files + deleted_files_for(package)
+        violations_for_files = package_violations_for(violations, files: files)
 
         # We `next false` because if we cannot find existing violations for `for_files` within
         # the `package_todo.yml` file, then there are no violations that
@@ -96,7 +100,14 @@ def delete_if_exists
 
     private
 
-    sig { params(package: String, violations: EntryType).returns(T::Boolean) }
+    sig { params(package: String).returns(T::Array[String]) }
+    def deleted_files_for(package)
+      old_files = old_entries.fetch(package, {}).values.flat_map { |violation| violation.fetch("files") }
+      new_files = @new_entries.fetch(package, {}).values.flat_map { |violation| violation.fetch("files") }
+      old_files - new_files
+    end
+
+    sig { params(package: String, violations: Entry).returns(T::Boolean) }
     def stale_violation_for_package?(package, violations:)
       violations.any? do |constant_name, entries_for_constant|
         new_entries_violation_types = T.cast(
@@ -118,10 +129,10 @@ def stale_violation_for_package?(package, violations:)
       end
     end
 
-    sig { params(package_violations: EntryType, files: T::Set[String]).returns(EntryType) }
+    sig { params(package_violations: Entry, files: T::Set[String]).returns(Entry) }
     def package_violations_for(package_violations, files:)
       {}.tap do |package_violations_for_files|
-        package_violations_for_files = T.cast(package_violations_for_files, EntryType)
+        package_violations_for_files = T.cast(package_violations_for_files, Entry)
 
         package_violations.each do |constant_name, entries_for_constant|
           entries_for_files = files & entries_for_constant.fetch("files")
@@ -135,7 +146,7 @@ def package_violations_for(package_violations, files:)
       end
     end
 
-    sig { returns(EntriesType) }
+    sig { returns(Entries) }
     def prepare_entries_for_dump
       @new_entries.each do |package_name, package_violations|
         package_violations.each do |_, entries_for_constant|
@@ -148,16 +159,16 @@ def prepare_entries_for_dump
       @new_entries = @new_entries.sort.to_h
     end
 
-    sig { returns(EntriesType) }
-    def todo_list
-      @todo_list ||= if File.exist?(@filepath)
+    sig { returns(Entries) }
+    def old_entries
+      @old_entries ||= if File.exist?(@filepath)
         load_yaml(@filepath)
       else
         {}
       end
     end
 
-    sig { params(filepath: String).returns(EntriesType) }
+    sig { params(filepath: String).returns(Entries) }
     def load_yaml(filepath)
       YAML.load_file(filepath) || {}
     rescue Psych::Exception
diff --git a/test/fixtures/package_todo.yml b/test/fixtures/package_todo.yml
index de8cfb1fc..98ac2cd92 100644
--- a/test/fixtures/package_todo.yml
+++ b/test/fixtures/package_todo.yml
@@ -5,4 +5,3 @@ buyers:
     - some_checker_type
     files:
     - orders/app/jobs/orders/sweepers/purge_old_document_rows_task.rb
-    - orders/app/models/orders/services/adjustment_engine.rb
diff --git a/test/unit/packwerk/package_todo_test.rb b/test/unit/packwerk/package_todo_test.rb
index 4b751087a..564245ea4 100644
--- a/test/unit/packwerk/package_todo_test.rb
+++ b/test/unit/packwerk/package_todo_test.rb
@@ -123,6 +123,15 @@ class PackageTodoTest < Minitest::Test
       )
     end
 
+    test "#stale_violations? returns true when deleted files are present" do
+      package = Package.new(name: "buyers", config: { "enforce_dependencies" => true })
+
+      package_todo = PackageTodo.new(package, "test/fixtures/package_todo.yml")
+      assert package_todo.stale_violations?(
+        Set.new(["orders/app/jobs/orders/sweepers/purge_old_document_rows_task.rb"])
+      )
+    end
+
     test "#listed? returns false if constant is not violated" do
       reference = build_reference(destination_package: destination_package)
       package_todo = PackageTodo.new(destination_package, "test/fixtures/package_todo.yml")
diff --git a/test/unit/packwerk/parse_run_test.rb b/test/unit/packwerk/parse_run_test.rb
index 3b0e1e2fe..d1c7d260d 100644
--- a/test/unit/packwerk/parse_run_test.rb
+++ b/test/unit/packwerk/parse_run_test.rb
@@ -328,11 +328,11 @@ def something
 
       expected_message = <<~EOS
         No offenses detected
-        No stale violations detected
+        There were stale violations found, please run `packwerk update-todo`
       EOS
       assert_equal expected_message, result.message
 
-      assert result.status
+      refute result.status
     end
 
     test "#check lists out violations of strict mode" do
@@ -435,7 +435,7 @@ def something
 
         1 offense detected
 
-        No stale violations detected
+        There were stale violations found, please run `packwerk update-todo`
       EOS
 
       assert_equal expected_message, result.message

From efa1a2a109efb8d330f889ad0655582b884710b8 Mon Sep 17 00:00:00 2001
From: Gannon McGibbon <gannon.mcgibbon@gmail.com>
Date: Tue, 14 Mar 2023 00:44:46 -0500
Subject: [PATCH 5/6] Make a private reader for @new_entries

---
 lib/packwerk/package_todo.rb | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/lib/packwerk/package_todo.rb b/lib/packwerk/package_todo.rb
index 5347a4e28..96855d498 100644
--- a/lib/packwerk/package_todo.rb
+++ b/lib/packwerk/package_todo.rb
@@ -41,7 +41,7 @@ def listed?(reference, violation_type:)
       params(reference: Packwerk::Reference, violation_type: String).returns(T::Boolean)
     end
     def add_entries(reference, violation_type)
-      package_violations = @new_entries.fetch(reference.constant.package.name, {})
+      package_violations = new_entries.fetch(reference.constant.package.name, {})
       entries_for_constant = package_violations[reference.constant.name] ||= {}
 
       entries_for_constant["violations"] ||= []
@@ -50,7 +50,7 @@ def add_entries(reference, violation_type)
       entries_for_constant["files"] ||= []
       entries_for_constant.fetch("files") << reference.relative_path.to_s
 
-      @new_entries[reference.constant.package.name] = package_violations
+      new_entries[reference.constant.package.name] = package_violations
       listed?(reference, violation_type: violation_type)
     end
 
@@ -73,7 +73,7 @@ def stale_violations?(for_files)
 
     sig { void }
     def dump
-      if @new_entries.empty?
+      if new_entries.empty?
         delete_if_exists
       else
         prepare_entries_for_dump
@@ -88,7 +88,7 @@ def dump
         MESSAGE
         File.open(@filepath, "w") do |f|
           f.write(message)
-          f.write(@new_entries.to_yaml)
+          f.write(new_entries.to_yaml)
         end
       end
     end
@@ -100,10 +100,13 @@ def delete_if_exists
 
     private
 
+    sig { returns(Entries) }
+    attr_reader(:new_entries)
+
     sig { params(package: String).returns(T::Array[String]) }
     def deleted_files_for(package)
       old_files = old_entries.fetch(package, {}).values.flat_map { |violation| violation.fetch("files") }
-      new_files = @new_entries.fetch(package, {}).values.flat_map { |violation| violation.fetch("files") }
+      new_files = new_entries.fetch(package, {}).values.flat_map { |violation| violation.fetch("files") }
       old_files - new_files
     end
 
@@ -111,17 +114,17 @@ def deleted_files_for(package)
     def stale_violation_for_package?(package, violations:)
       violations.any? do |constant_name, entries_for_constant|
         new_entries_violation_types = T.cast(
-          @new_entries.dig(package, constant_name, "violations"),
+          new_entries.dig(package, constant_name, "violations"),
           T.nilable(T::Array[String]),
         )
         # If there are no NEW entries that match the old entries `for_files`,
-        # @new_entries is from the list of violations we get when we check this file.
+        # new_entries is from the list of violations we get when we check this file.
         # If this list is empty, we also must have stale violations.
         next true if new_entries_violation_types.nil?
 
         if entries_for_constant.fetch("violations").all? { |type| new_entries_violation_types.include?(type) }
           stale_violations =
-            entries_for_constant.fetch("files") - Array(@new_entries.dig(package, constant_name, "files"))
+            entries_for_constant.fetch("files") - Array(new_entries.dig(package, constant_name, "files"))
           stale_violations.any?
         else
           return true
@@ -148,15 +151,15 @@ def package_violations_for(package_violations, files:)
 
     sig { returns(Entries) }
     def prepare_entries_for_dump
-      @new_entries.each do |package_name, package_violations|
+      new_entries.each do |package_name, package_violations|
         package_violations.each do |_, entries_for_constant|
           entries_for_constant.fetch("violations").sort!.uniq!
           entries_for_constant.fetch("files").sort!.uniq!
         end
-        @new_entries[package_name] = package_violations.sort.to_h
+        new_entries[package_name] = package_violations.sort.to_h
       end
 
-      @new_entries = @new_entries.sort.to_h
+      @new_entries = new_entries.sort.to_h
     end
 
     sig { returns(Entries) }

From 664b2f543d4dc7a4cd8f393e9d0638f0a37df248 Mon Sep 17 00:00:00 2001
From: Gannon McGibbon <gannon.mcgibbon@gmail.com>
Date: Tue, 14 Mar 2023 00:58:20 -0500
Subject: [PATCH 6/6] Simplify path handling in PackageTodo

---
 lib/packwerk/package_todo.rb | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/lib/packwerk/package_todo.rb b/lib/packwerk/package_todo.rb
index 96855d498..c6bbed5cb 100644
--- a/lib/packwerk/package_todo.rb
+++ b/lib/packwerk/package_todo.rb
@@ -15,10 +15,10 @@ class PackageTodo
       T::Hash[PackageName, Entry]
     end
 
-    sig { params(package: Packwerk::Package, filepath: String).void }
-    def initialize(package, filepath)
+    sig { params(package: Packwerk::Package, path: String).void }
+    def initialize(package, path)
       @package = package
-      @filepath = filepath
+      @path = path
       @new_entries = T.let({}, Entries)
       @old_entries = T.let(nil, T.nilable(Entries))
     end
@@ -86,7 +86,7 @@ def dump
           #
           # bin/packwerk update-todo
         MESSAGE
-        File.open(@filepath, "w") do |f|
+        File.open(@path, "w") do |f|
           f.write(message)
           f.write(new_entries.to_yaml)
         end
@@ -95,7 +95,7 @@ def dump
 
     sig { void }
     def delete_if_exists
-      File.delete(@filepath) if File.exist?(@filepath)
+      File.delete(@path) if File.exist?(@path)
     end
 
     private
@@ -164,16 +164,12 @@ def prepare_entries_for_dump
 
     sig { returns(Entries) }
     def old_entries
-      @old_entries ||= if File.exist?(@filepath)
-        load_yaml(@filepath)
-      else
-        {}
-      end
+      @old_entries ||= load_yaml_file(@path)
     end
 
-    sig { params(filepath: String).returns(Entries) }
-    def load_yaml(filepath)
-      YAML.load_file(filepath) || {}
+    sig { params(path: String).returns(Entries) }
+    def load_yaml_file(path)
+      File.exist?(path) && YAML.load_file(path) || {}
     rescue Psych::Exception
       {}
     end