Fix: Handle renamed files in package_todo.yml validation (#59)

Author: martinemde

lib/danger-packwerk/private/todo_yml_changes.rb

@@ -33,15 +33,23 @@ def self.get_reference_offenses(violation_types, git_filesystem)
         renamed_files_before = git_filesystem.renamed_files.map { |before_after_file| before_after_file[:before] }
         renamed_files_after = git_filesystem.renamed_files.map { |before_after_file| before_after_file[:after] }
 
+        # Build a rename mapping to normalize file paths when comparing violations
+        rename_mapping = build_rename_mapping(git_filesystem.renamed_files)
+
         git_filesystem.modified_files.grep(PACKAGE_TODO_PATTERN).each do |modified_package_todo_yml_file|
           # We skip over modified files if one of the modified files is a renamed `package_todo.yml` file.
           # This allows us to rename packs while ignoring "new violations" in those renamed packs.
           next if renamed_files_before.include?(modified_package_todo_yml_file)
 
           head_commit_violations = BasicReferenceOffense.from(modified_package_todo_yml_file)
           base_commit_violations = get_violations_before_patch_for(git_filesystem, modified_package_todo_yml_file)
-          added_violations += head_commit_violations - base_commit_violations
-          removed_violations += base_commit_violations - head_commit_violations
+
+          # Normalize violations for renames: update old file paths to new file paths
+          # so that violations referring to renamed files are properly matched
+          normalized_base_violations = normalize_violations_for_renames(base_commit_violations, rename_mapping)
+
+          added_violations += head_commit_violations - normalized_base_violations
+          removed_violations += normalized_base_violations - head_commit_violations
         end
 
         #
@@ -78,6 +86,40 @@ def self.get_reference_offenses(violation_types, git_filesystem)
         [relevant_added_violations, relevant_removed_violations]
       end
 
+      sig do
+        params(
+          renamed_files: T::Array[{ after: String, before: String }]
+        ).returns(T::Hash[String, String])
+      end
+      def self.build_rename_mapping(renamed_files)
+        renamed_files.each_with_object({}) do |rename, mapping|
+          mapping[rename[:before]] = rename[:after]
+        end
+      end
+
+      sig do
+        params(
+          violations: T::Array[BasicReferenceOffense],
+          rename_mapping: T::Hash[String, String]
+        ).returns(T::Array[BasicReferenceOffense])
+      end
+      def self.normalize_violations_for_renames(violations, rename_mapping)
+        violations.map do |violation|
+          file = rename_mapping[violation.file]
+          next violation unless file
+
+          # Create a new violation with the updated file path
+          BasicReferenceOffense.new(
+            class_name: violation.class_name,
+            file: file,
+            to_package_name: violation.to_package_name,
+            from_package_name: violation.from_package_name,
+            type: violation.type,
+            file_location: violation.file_location
+          )
+        end
+      end
+
       sig do
         params(
           git_filesystem: GitFilesystem,

spec/danger_packwerk/danger_package_todo_yml_changes_spec.rb

@@ -914,6 +914,74 @@ def format_offenses(added_violations, repo_link, org_name, repo_url_builder: nil
           subject
           expect(dangerfile).to produce_no_danger_messages
         end
+
+        it 'correctly normalizes file paths when comparing violations' do
+          expect(slack_notifier).to receive(:notify_slack).with(
+            { dependency: { minus: 0, plus: 0 }, privacy: { minus: 0, plus: 0 } },
+            ['packs/some_pack/package_todo.yml']
+          )
+          subject
+        end
+      end
+
+      context 'a package_todo.yml file has been modified with multiple files that have been renamed' do
+        let(:renamed_files) do
+          [
+            {
+              after: 'packs/some_pack/some_class_with_new_name.rb',
+              before: 'packs/some_pack/some_class_with_old_name.rb'
+            },
+            {
+              after: 'packs/some_pack/another_renamed_class.rb',
+              before: 'packs/some_pack/another_old_name.rb'
+            }
+          ]
+        end
+
+        let(:modified_files) do
+          [
+            write_file('packs/some_pack/package_todo.yml', <<~YML.strip)
+              ---
+              packs/some_other_pack:
+                "OtherPackClass":
+                  violations:
+                  - privacy
+                  files:
+                  - packs/some_pack/some_class_with_new_name.rb
+                  - packs/some_pack/another_renamed_class.rb
+            YML
+          ]
+        end
+
+        let(:some_pack_package_todo_before) do
+          write_file('packs/some_pack/package_todo.yml', <<~YML.strip)
+            ---
+            packs/some_other_pack:
+              "OtherPackClass":
+                violations:
+                - privacy
+                files:
+                - packs/some_pack/some_class_with_old_name.rb
+                - packs/some_pack/another_old_name.rb
+          YML
+        end
+
+        before do
+          ['packs/some_pack/another_renamed_class.rb', 'packs/some_pack/another_old_name.rb'].each { |path| write_file(path) }
+        end
+
+        it 'does not display a markdown message' do
+          subject
+          expect(dangerfile).to produce_no_danger_messages
+        end
+
+        it 'correctly normalizes all renamed file paths when comparing violations' do
+          expect(slack_notifier).to receive(:notify_slack).with(
+            { dependency: { minus: 0, plus: 0 }, privacy: { minus: 0, plus: 0 } },
+            ['packs/some_pack/package_todo.yml']
+          )
+          subject
+        end
       end
 
       context 'a package_todo.yml file has been modified with files that have been renamed AND been added' do