Revert "switch to simpler implementation"

This reverts commit 0a72074.

lib/danger-packwerk/basic_reference_offense.rb

@@ -38,6 +38,7 @@ def hash
     const :file, String
     const :to_package_name, String
     const :type, String
+    const :class_name_location, Location
     const :file_location, Location
 
     sig { params(deprecated_references_yml: String).returns(T::Array[BasicReferenceOffense]) }
@@ -84,19 +85,22 @@ def self.from(deprecated_references_yml)
           raise "Unable to find reference to violation #{debug_info} in #{deprecated_references_yml}"
         end
 
+        # We add one to the line number since `each_with_index` is zero-based indexed but Github line numbers are one-based indexed
+        class_name_location = Location.new(file: deprecated_references_yml, line_number: class_name_line_number + 1)
+
         violation.files.map do |file|
           file_line_numbers = file_reference_to_line_number_index.fetch(file, [])
           file_line_number = file_line_numbers.select { |index| index > class_name_line_number }.min
           raise "Unable to find reference to violation #{{ file: file, to_package_name: violation.to_package_name, type: violation.type }} in #{deprecated_references_yml}" if file_line_number.nil?
 
-          # We add one to the line number since `each_with_index` is zero-based indexed but Github line numbers are one-based indexed
           file_location = Location.new(file: deprecated_references_yml, line_number: file_line_number + 1)
 
           BasicReferenceOffense.new(
             class_name: violation.class_name,
             file: file,
             to_package_name: violation.to_package_name,
             type: violation.type,
+            class_name_location: class_name_location,
             file_location: file_location
           )
         end

lib/danger-packwerk/danger_deprecated_references_yml_changes.rb

@@ -45,10 +45,19 @@ def check(
 
       current_comment_count = 0
 
-      violation_diff.added_violations.group_by(&:class_name).each do |_class_name, violations|
+      violation_diff.added_violations.group_by(&:class_name).each do |class_name, violations|
         break if current_comment_count >= max_comments
 
-        location = T.must(violations.first).file_location
+        # If we already had a violation on this constant, then the constant may not be in the visible diff,
+        # so we drop the comment on the reference to the file, which will always be in the diff.
+        # Note that we could also choose to *always* leave all comments on the first reference to the file, which would
+        # simplify our internal implementation (as we only have one place to put it, so we don't need to check `all_violations_before` at all).
+        # This might be best because then we don't need to expose more API on `ViolationDiff`.
+        location = if violation_diff.all_violations_before.any? { |v| v.class_name == class_name }
+                     T.must(violations.first).file_location
+                   else
+                     T.must(violations.first).class_name_location
+                   end
 
         markdown(
           added_offenses_formatter.call(violations),
@@ -64,6 +73,7 @@ def check(
     def get_violation_diff # rubocop:disable Naming/AccessorMethodName
       added_violations = T.let([], T::Array[BasicReferenceOffense])
       removed_violations = T.let([], T::Array[BasicReferenceOffense])
+      all_violations_before = T.let([], T::Array[BasicReferenceOffense])
 
       git.added_files.grep(DEPRECATED_REFERENCES_PATTERN).each do |added_deprecated_references_yml_file|
         # Since the file is added, we know on the base commit there are no violations related to this pack,
@@ -75,19 +85,22 @@ def get_violation_diff # rubocop:disable Naming/AccessorMethodName
         # Since the file is deleted, we know on the HEAD commit there are no violations related to this pack,
         # and that all violations from this file are deleted
         deleted_violations = get_violations_before_patch_for(deleted_deprecated_references_yml_file)
+        all_violations_before += deleted_violations
         removed_violations += deleted_violations
       end
 
       git.modified_files.grep(DEPRECATED_REFERENCES_PATTERN).each do |modified_deprecated_references_yml_file|
         head_commit_violations = BasicReferenceOffense.from(modified_deprecated_references_yml_file)
         base_commit_violations = get_violations_before_patch_for(modified_deprecated_references_yml_file)
+        all_violations_before += base_commit_violations
         added_violations += head_commit_violations - base_commit_violations
         removed_violations += base_commit_violations - head_commit_violations
       end
 
       ViolationDiff.new(
         added_violations: added_violations,
-        removed_violations: removed_violations
+        removed_violations: removed_violations,
+        all_violations_before: all_violations_before
       )
     end
 

lib/danger-packwerk/violation_diff.rb

@@ -9,5 +9,6 @@ class ViolationDiff < T::Struct
 
     const :added_violations, T::Array[BasicReferenceOffense]
     const :removed_violations, T::Array[BasicReferenceOffense]
+    const :all_violations_before, T::Array[BasicReferenceOffense]
   end
 end

spec/danger_packwerk/danger_deprecated_references_yml_changes_spec.rb

@@ -92,7 +92,7 @@ module DangerPackwerk
             EXPECTED
 
             expect(actual_markdown.message).to eq expected
-            expect(actual_markdown.line).to eq 8
+            expect(actual_markdown.line).to eq 3
             expect(actual_markdown.file).to eq 'packs/some_pack/deprecated_references.yml'
             expect(actual_markdown.type).to eq :markdown
           end
@@ -134,7 +134,7 @@ module DangerPackwerk
             EXPECTED
 
             expect(actual_markdown.message).to eq expected
-            expect(actual_markdown.line).to eq 8
+            expect(actual_markdown.line).to eq 3
             expect(actual_markdown.file).to eq 'packs/some_pack/deprecated_references.yml'
             expect(actual_markdown.type).to eq :markdown
           end
@@ -280,7 +280,7 @@ module DangerPackwerk
           EXPECTED
 
           expect(actual_markdown.message).to eq expected
-          expect(actual_markdown.line).to eq 13
+          expect(actual_markdown.line).to eq 8
           expect(actual_markdown.file).to eq 'packs/some_pack/deprecated_references.yml'
           expect(actual_markdown.type).to eq :markdown
         end