Clarify update_todo command error message to guide users better

Shopify · Aug 5, 2023 · 8b23021 · 8b23021
1 parent a03e83a
commit 8b23021
Show file tree

Hide file tree

Showing 3 changed files with 22 additions and 12 deletions.
diff --git a/lib/packwerk/commands/update_todo_command.rb b/lib/packwerk/commands/update_todo_command.rb
@@ -39,12 +39,21 @@ def run
         offense_collection.add_offenses(offenses)
         offense_collection.persist_package_todo_files(run_context.package_set)
 
-        out.puts(<<~EOS)
-          #{offenses_formatter.show_offenses(offense_collection.errors + offense_collection.strict_mode_violations)}
-          ✅ `package_todo.yml` has been updated.
-        EOS
+        unlisted_strict_mode_violations = offense_collection.unlisted_strict_mode_violations
 
-        offense_collection.strict_mode_violations.empty? && offense_collection.errors.empty?
+        messages = [
+          offenses_formatter.show_offenses(offense_collection.errors + unlisted_strict_mode_violations),
+        ]
+
+        messages << if unlisted_strict_mode_violations.any?
+          "⚠️ `package_todo.yml` has been updated, but unlisted strict mode violations were not added."
+        else
+          "✅ `package_todo.yml` has been updated."
+        end
+
+        out.puts(messages.select(&:present?).join("\n") + "\n")
+
+        unlisted_strict_mode_violations.empty? && offense_collection.errors.empty?
       end
     end
   end

diff --git a/lib/packwerk/offense_collection.rb b/lib/packwerk/offense_collection.rb
@@ -38,8 +38,7 @@ def initialize(root_path, package_todos = {})
     def listed?(offense)
       return false unless offense.is_a?(ReferenceOffense)
 
-      reference = offense.reference
-      package_todo_for(reference.package).listed?(reference, violation_type: offense.violation_type)
+      already_listed?(offense)
     end
 
     sig { params(offenses: T::Array[Offense]).void }
@@ -86,6 +85,11 @@ def outstanding_offenses
       errors + new_violations
     end
 
+    sig { returns(T::Array[Packwerk::ReferenceOffense]) }
+    def unlisted_strict_mode_violations
+      strict_mode_violations.reject { |offense| already_listed?(offense) }
+    end
+
     private
 
     sig { params(offense: ReferenceOffense).returns(T::Boolean) }

diff --git a/test/unit/packwerk/commands/update_todo_command_test.rb b/test/unit/packwerk/commands/update_todo_command_test.rb
@@ -126,15 +126,12 @@ class UpdateTodoCommandTest < Minitest::Test
 
           📦 Finished in \\d+\\.\\d+ seconds
 
-          components/source/some/path.rb
-          some message
-
           components/source/other/path.rb
           other message
 
-          2 offenses detected
+          1 offense detected
 
-          ✅ `package_todo.yml` has been updated.
+          ⚠️ `package_todo.yml` has been updated, but unlisted strict mode violations were not added.
         EOS
         assert_match(/#{expected_output}/, out.string)