Fix UX consistency

Gemfile.lock

@@ -1,7 +1,7 @@
 PATH
   remote: .
   specs:
-    danger-packwerk (0.1.0)
+    danger-packwerk (0.1.1)
       danger-plugin-api (~> 1.0)
       packwerk
       sorbet-runtime
@@ -44,7 +44,7 @@ GEM
     cork (0.3.0)
       colored2 (~> 3.1)
     crass (1.0.6)
-    danger (8.5.0)
+    danger (8.6.0)
       claide (~> 1.0)
       claide-plugins (>= 0.9.2)
       colored2 (~> 3.1)
@@ -87,7 +87,7 @@ GEM
     faraday-patron (1.0.0)
     faraday-rack (1.0.0)
     faraday-retry (1.0.3)
-    git (1.10.2)
+    git (1.11.0)
       rchardet (~> 1.8)
     html_tokenizer (0.0.7)
     i18n (1.10.0)
@@ -105,7 +105,7 @@ GEM
     multipart-post (2.1.1)
     nap (1.1.0)
     no_proxy_fix (0.1.2)
-    nokogiri (1.13.3)
+    nokogiri (1.13.4)
       mini_portile2 (~> 2.8.0)
       racc (~> 1.4)
     octokit (4.22.0)
@@ -128,7 +128,7 @@ GEM
     pry (0.14.1)
       coderay (~> 1.1)
       method_source (~> 1.0)
-    public_suffix (4.0.6)
+    public_suffix (4.0.7)
     racc (1.6.0)
     rails-dom-testing (2.0.3)
       activesupport (>= 4.2.0)

lib/danger-packwerk/basic_reference_offense.rb

@@ -38,38 +38,66 @@ def hash
     const :file, String
     const :to_package_name, String
     const :type, String
-    const :location, Location
+    const :file_location, Location
 
     sig { params(deprecated_references_yml: String).returns(T::Array[BasicReferenceOffense]) }
     def self.from(deprecated_references_yml)
       deprecated_references_yml_pathname = Pathname.new(deprecated_references_yml)
       violations = Private::DeprecatedReferences.from(deprecated_references_yml_pathname).violations
 
+      # See the larger comment below for more information on why we need this information.
+      # This is a small optimization that lets us find the location of referenced files within
+      # a `deprecated_references.yml` file. Getting this now allows us to avoid reading through the file
+      # once for every referenced file in the inner loop below.
+      file_reference_to_line_number_index = T.let({}, T::Hash[String, T::Array[Integer]])
+      all_referenced_files = violations.flat_map(&:files).uniq
+      deprecated_references_yml_pathname.readlines.each_with_index do |line, index|
+        # We can use `find` here to exit early since each line will include one path that is unique to that file.
+        # Paths should not be substrings of each other, since they are all paths relative to the root.
+        file_on_line = all_referenced_files.find { |file| line.include?(file) }
+        # Not all lines contain a reference to a file
+        if file_on_line
+          file_reference_to_line_number_index[file_on_line] ||= []
+          file_reference_to_line_number_index.fetch(file_on_line) << index
+        end
+      end
+
       violations.flat_map do |violation|
-        # We choose the location of the violation as the reference to the constant within the `deprecated_references.yml` file.
-        # We simply find the reference to that constant, because we know that each constant reference can occur only once per `deprecated_references.yml` file
+        #
+        # We identify two locations associated with this violation.
+        # First, we find the reference to the constant within the `deprecated_references.yml` file.
+        # We know that each constant reference can occur only once per `deprecated_references.yml` file
         # The reason for this is that we know that only one file in the codebase can define a constant, and packwerk's constant_resolver will actually
         # raise if this assumption is not true: https://github.com/Shopify/constant_resolver/blob/e78af0c8d5782b06292c068cfe4176e016c51b34/lib/constant_resolver.rb#L74
         #
+        # Second, we find the reference to the specific file that references the constant within the `deprecated_references.yml` file.
+        # This can occur multiple times per `deprecated_references.yml` file, but we know that the very first reference to the file after the class name key will be the one we care
+        # about, so we take the first instance that occurs after the class is listed.
+        #
         # Note though that since one constant reference in a `deprecated_referencs.yml` can be both a privacy and a dependency violation AND it can occur in many files,
         # we need to group them. That is -- if `MyPrivateConstant` is both a dependency and a privacy violation AND it occurs in 10 files, that would represent 20 violations.
         # Therefore we will group all of those 20 into one message to the user rather than providing 20 messages.
-        _line, line_number = deprecated_references_yml_pathname.readlines.each_with_index.find { |line, _index| line.include?(violation.class_name) }
-        if line_number.nil?
+        #
+        _line, class_name_line_number = deprecated_references_yml_pathname.readlines.each_with_index.find { |line, _index| line.include?(violation.class_name) }
+        if class_name_line_number.nil?
           debug_info = { class_name: violation.class_name, to_package_name: violation.to_package_name, type: violation.type }
           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
-        location = Location.new(file: deprecated_references_yml, line_number: 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,
-            location: location
+            file_location: file_location
           )
         end
       end

lib/danger-packwerk/danger_deprecated_references_yml_changes.rb

@@ -44,9 +44,12 @@ def check(
       )
 
       current_comment_count = 0
-      violation_diff.added_violations.group_by(&:location).each do |location, 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
+
         markdown(
           added_offenses_formatter.call(violations),
           line: location.line_number,
@@ -71,13 +74,13 @@ def get_violation_diff # rubocop:disable Naming/AccessorMethodName
       git.deleted_files.grep(DEPRECATED_REFERENCES_PATTERN).each do |deleted_deprecated_references_yml_file|
         # 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
-        removed_violations += get_violations_before_patch_for(deleted_deprecated_references_yml_file)
+        deleted_violations = get_violations_before_patch_for(deleted_deprecated_references_yml_file)
+        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)
-
         added_violations += head_commit_violations - base_commit_violations
         removed_violations += base_commit_violations - head_commit_violations
       end

lib/danger-packwerk/version.rb

@@ -2,5 +2,5 @@
 # frozen_string_literal: true
 
 module DangerPackwerk
-  VERSION = '0.1.0'
+  VERSION = '0.1.1'
 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
@@ -337,15 +337,15 @@ module DangerPackwerk
               ---
               packs/some_other_pack:
                 "OtherPackClass":
-              ==================== DANGER_START
-              Hi! It looks like the pack defining `OtherPackClass` considers this private API.
-              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 this violation?
-              ==================== DANGER_END
                   violations:
                   - privacy
                   files:
                   - packs/some_pack/some_class.rb
                   - packs/some_pack/some_other_class.rb
+              ==================== DANGER_START
+              Hi! It looks like the pack defining `OtherPackClass` considers this private API.
+              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 this violation?
+              ==================== DANGER_END
             EXPECTED
           ).and_nothing_else
         end
@@ -414,15 +414,15 @@ module DangerPackwerk
                   - packs/some_pack/some_class.rb
                   - packs/some_pack/some_other_class.rb
                 "SomeOtherPackClass":
-              ==================== DANGER_START
-              Hi! It looks like the pack defining `SomeOtherPackClass` considers this private API.
-              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 this violation?
-              ==================== DANGER_END
                   violations:
                   - privacy
                   files:
                   - packs/some_pack/some_class.rb
                   - packs/some_pack/some_other_class.rb
+              ==================== DANGER_START
+              Hi! It looks like the pack defining `SomeOtherPackClass` considers this private API.
+              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 this violation?
+              ==================== DANGER_END
             EXPECTED
           ).and_nothing_else
         end
@@ -491,15 +491,15 @@ module DangerPackwerk
               ---
               packs/some_other_pack:
                 "OtherPackClass":
-              ==================== DANGER_START
-              Hi! It looks like the pack defining `OtherPackClass` is 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 this violation?
-              ==================== DANGER_END
                   violations:
                   - privacy
                   - dependency
                   files:
                   - packs/some_pack/some_class.rb
+              ==================== DANGER_START
+              Hi! It looks like the pack defining `OtherPackClass` is 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 this violation?
+              ==================== DANGER_END
             EXPECTED
           ).and_nothing_else
         end
@@ -598,15 +598,15 @@ module DangerPackwerk
               ---
               packs/some_other_pack:
                 "OtherPackClass":
-              ==================== DANGER_START
-              Hi! It looks like the pack defining `OtherPackClass` is 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_class1.rb
+              ==================== DANGER_START
+              Hi! It looks like the pack defining `OtherPackClass` is 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
                   - packs/some_pack/some_class2.rb
                   - packs/some_pack/some_class3.rb
                   - packs/some_pack/some_class4.rb