Revert "switch to simpler implementation"

This reverts commit d91492e.

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

@@ -85,15 +85,15 @@ module DangerPackwerk
                 ---
                 packs/some_other_pack:
                   "OtherPackClass":
+                ==================== DANGER_START
+                Hi! It looks like the pack defining `OtherPackClass` considers this private API, and it's also not in the referencing pack's list of dependencies.
+                We noticed you ran `bin/packwerk update-deprecations`. Make sure to read through [the docs](https://github.com/Shopify/packwerk/blob/b647594f93c8922c038255a7aaca125d391a1fbf/docs/new_violation_flow_chart.pdf) for other ways to resolve. Could you add some context as a reply here about why we needed to add these violations?
+                ==================== DANGER_END
                     violations:
                     - privacy
                     - dependency
                     files:
                     - packs/some_pack/some_class.rb
-                ==================== DANGER_START
-                Hi! It looks like the pack defining `OtherPackClass` considers this private API, and it's also not in the referencing pack's list of dependencies.
-                We noticed you ran `bin/packwerk update-deprecations`. Make sure to read through [the docs](https://github.com/Shopify/packwerk/blob/b647594f93c8922c038255a7aaca125d391a1fbf/docs/new_violation_flow_chart.pdf) for other ways to resolve. Could you add some context as a reply here about why we needed to add these violations?
-                ==================== DANGER_END
               EXPECTED
             ).and_nothing_else
           end
@@ -128,17 +128,17 @@ module DangerPackwerk
                 ---
                 packs/some_other_pack:
                   "OtherPackClass":
-                    violations:
-                    - privacy
-                    - dependency
-                    files:
-                    - packs/some_pack/some_class.rb
                 ==================== DANGER_START
                 There are 2 new violations,
                 with class_names ["OtherPackClass"],
                 with to_package_names ["packs/some_other_pack"],
                 with types ["dependency", "privacy"],
                 ==================== DANGER_END
+                    violations:
+                    - privacy
+                    - dependency
+                    files:
+                    - packs/some_pack/some_class.rb
               EXPECTED
             ).and_nothing_else
           end
@@ -278,15 +278,15 @@ module DangerPackwerk
                   files:
                   - packs/some_pack/some_class.rb
                 "OtherPackClass2":
+              ==================== DANGER_START
+              Hi! It looks like the pack defining `OtherPackClass2` considers this private API, and it's also not in the referencing pack's list of dependencies.
+              We noticed you ran `bin/packwerk update-deprecations`. Make sure to read through [the docs](https://github.com/Shopify/packwerk/blob/b647594f93c8922c038255a7aaca125d391a1fbf/docs/new_violation_flow_chart.pdf) for other ways to resolve. Could you add some context as a reply here about why we needed to add these violations?
+              ==================== DANGER_END
                   violations:
                   - privacy
                   - dependency
                   files:
                   - packs/some_pack/some_class.rb
-              ==================== DANGER_START
-              Hi! It looks like the pack defining `OtherPackClass2` considers this private API, and it's also not in the referencing pack's list of dependencies.
-              We noticed you ran `bin/packwerk update-deprecations`. Make sure to read through [the docs](https://github.com/Shopify/packwerk/blob/b647594f93c8922c038255a7aaca125d391a1fbf/docs/new_violation_flow_chart.pdf) for other ways to resolve. Could you add some context as a reply here about why we needed to add these violations?
-              ==================== DANGER_END
             EXPECTED
           ).and_nothing_else
         end