From 4828e644905c9f50bcc367b0452e5ed91df370f4 Mon Sep 17 00:00:00 2001 From: Martin Emde Date: Mon, 13 Oct 2025 17:30:08 -0700 Subject: [PATCH 1/2] Fix: Handle renamed files in package_todo.yml validation --- .../private/todo_yml_changes.rb | 48 ++++++++++++- .../danger_package_todo_yml_changes_spec.rb | 68 +++++++++++++++++++ 2 files changed, 114 insertions(+), 2 deletions(-) diff --git a/lib/danger-packwerk/private/todo_yml_changes.rb b/lib/danger-packwerk/private/todo_yml_changes.rb index 8e25a21..cd19075 100644 --- a/lib/danger-packwerk/private/todo_yml_changes.rb +++ b/lib/danger-packwerk/private/todo_yml_changes.rb @@ -33,6 +33,9 @@ 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. @@ -40,8 +43,13 @@ def self.get_reference_offenses(violation_types, git_filesystem) 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,42 @@ 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| + if rename_mapping.key?(violation.file) + # Create a new violation with the updated file path + new_file_path = T.must(rename_mapping[violation.file]) + BasicReferenceOffense.new( + class_name: violation.class_name, + file: new_file_path, + to_package_name: violation.to_package_name, + from_package_name: violation.from_package_name, + type: violation.type, + file_location: violation.file_location + ) + else + violation + end + end + end + sig do params( git_filesystem: GitFilesystem, diff --git a/spec/danger_packwerk/danger_package_todo_yml_changes_spec.rb b/spec/danger_packwerk/danger_package_todo_yml_changes_spec.rb index a91ce9a..b393a85 100644 --- a/spec/danger_packwerk/danger_package_todo_yml_changes_spec.rb +++ b/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 From 215a3c276863edba14060f65c22ed3c63b56e582 Mon Sep 17 00:00:00 2001 From: Martin Emde Date: Tue, 21 Oct 2025 10:57:26 -0700 Subject: [PATCH 2/2] remove t.must --- .../private/todo_yml_changes.rb | 26 +++++++++---------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/lib/danger-packwerk/private/todo_yml_changes.rb b/lib/danger-packwerk/private/todo_yml_changes.rb index cd19075..0b01b6b 100644 --- a/lib/danger-packwerk/private/todo_yml_changes.rb +++ b/lib/danger-packwerk/private/todo_yml_changes.rb @@ -105,20 +105,18 @@ def self.build_rename_mapping(renamed_files) end def self.normalize_violations_for_renames(violations, rename_mapping) violations.map do |violation| - if rename_mapping.key?(violation.file) - # Create a new violation with the updated file path - new_file_path = T.must(rename_mapping[violation.file]) - BasicReferenceOffense.new( - class_name: violation.class_name, - file: new_file_path, - to_package_name: violation.to_package_name, - from_package_name: violation.from_package_name, - type: violation.type, - file_location: violation.file_location - ) - else - violation - end + 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