Simplify offense data model by removing adapter pattern (#63)

Remove the adapter classes (PackageAdapter, ConstantAdapter,
ReferenceAdapter, LocationAdapter) from PksOffense that existed solely
to make it compatible with Packwerk::ReferenceOffense interface.

Instead, use PksOffense's direct properties throughout:
- offense.constant_name instead of offense.reference.constant.name
- offense.file instead of offense.reference.relative_path
- offense.line instead of offense.location.line
- offense.defining_pack_name instead of offense.reference.constant.package.name

Update Check::OffensesFormatter interface to accept PksOffense directly
instead of Packwerk::ReferenceOffense, and update DefaultFormatter to
resolve constant locations via Private.constant_resolver.

Closes dp-5q5

Author: martinemde

lib/danger-packwerk/check/default_formatter.rb

@@ -19,23 +19,23 @@ def initialize(custom_help_message: nil)
 
       sig do
         override.params(
-          offenses: T::Array[Packwerk::ReferenceOffense],
+          offenses: T::Array[PksOffense],
           repo_link: String,
           org_name: String,
           repo_url_builder: T.nilable(T.proc.params(constant_path: String).returns(String))
         ).returns(String)
       end
       def format_offenses(offenses, repo_link, org_name, repo_url_builder: nil)
-        reference_offense = T.must(offenses.first)
+        offense = T.must(offenses.first)
         violation_types = offenses.map(&:violation_type)
-        referencing_file = reference_offense.reference.relative_path
+        referencing_file = offense.file
         referencing_file_pack = ParsePackwerk.package_from_path(referencing_file).name
         # We remove leading double colons as they feel like an implementation detail of packwerk.
-        constant_name = reference_offense.reference.constant.name.delete_prefix('::')
+        constant_name = offense.constant_name.delete_prefix('::')
 
-        constant_source_package_name = reference_offense.reference.constant.package.name
+        constant_source_package_name = offense.defining_pack_name
 
-        constant_location = reference_offense.reference.constant.location
+        constant_location = Private.constant_resolver.resolve(offense.constant_name)&.location || offense.file
         constant_source_package = T.must(ParsePackwerk.all.find { |p| p.name == constant_source_package_name })
         constant_source_package_ownership_info = Private::OwnershipInformation.for_package(constant_source_package, org_name)
 

lib/danger-packwerk/check/offenses_formatter.rb

@@ -12,7 +12,7 @@ module OffensesFormatter
 
       sig do
         abstract.params(
-          offenses: T::Array[Packwerk::ReferenceOffense],
+          offenses: T::Array[PksOffense],
           repo_link: String,
           org_name: String,
           repo_url_builder: T.nilable(T.proc.params(constant_path: String).returns(String))

lib/danger-packwerk/danger_packwerk.rb

@@ -20,8 +20,7 @@ class DangerPackwerk < Danger::Plugin
     # especially given all violations should fail the build anyways.
     # We set a max (rather than unlimited) to avoid GitHub rate limiting and general spam if a PR does some sort of mass rename.
     DEFAULT_MAX_COMMENTS = 15
-    # Support both legacy Packwerk::ReferenceOffense and new PksOffense types
-    OnFailure = T.type_alias { T.proc.params(offenses: T::Array[T.any(Packwerk::ReferenceOffense, PksOffense)]).void }
+    OnFailure = T.type_alias { T.proc.params(offenses: T::Array[PksOffense]).void }
     DEFAULT_ON_FAILURE = T.let(->(offenses) {}, OnFailure)
     DEFAULT_FAIL = false
     DEFAULT_FAILURE_MESSAGE = 'Packwerk violations were detected! Please resolve them to unblock the build.'
@@ -109,8 +108,7 @@ def check(
       renamed_files = git_filesystem.renamed_files.map { |before_after_file| before_after_file[:after] }
 
       offenses_to_care_about = pks_offenses.reject do |offense|
-        constant_name = offense.reference.constant.name
-        filepath_that_defines_this_constant = Private.constant_resolver.resolve(constant_name)&.location
+        filepath_that_defines_this_constant = Private.constant_resolver.resolve(offense.constant_name)&.location
         # Ignore references that have been renamed
         renamed_files.include?(filepath_that_defines_this_constant) ||
           # Ignore violations that are not in the allow-list of violation types to leave comments for
@@ -123,14 +121,14 @@ def check(
         case grouping_strategy
         when CommentGroupingStrategy::PerConstantPerLocation
           [
-            offense.reference.constant.name,
-            offense.location.line,
-            offense.reference.relative_path
+            offense.constant_name,
+            offense.line,
+            offense.file
           ]
         when CommentGroupingStrategy::PerConstantPerPack
           [
-            offense.reference.constant.name,
-            ParsePackwerk.package_from_path(offense.reference.relative_path)
+            offense.constant_name,
+            ParsePackwerk.package_from_path(offense.file)
           ]
         else
           T.absurd(grouping_strategy)
@@ -141,11 +139,10 @@ def check(
         current_comment_count += 1
 
         offense = T.must(unique_offenses.first)
-        line_number = offense.location.line
-        referencing_file = offense.reference.relative_path
+        line_number = offense.line
+        referencing_file = offense.file
 
-        # PksOffense adapters provide Packwerk::ReferenceOffense-compatible interface
-        message = offenses_formatter.format_offenses(T.unsafe(unique_offenses), repo_link, org_name, repo_url_builder: repo_url_builder)
+        message = offenses_formatter.format_offenses(unique_offenses, repo_link, org_name, repo_url_builder: repo_url_builder)
         markdown(message, file: git_filesystem.convert_to_filesystem(referencing_file), line: line_number)
       end
 

lib/danger-packwerk/pks_offense.rb

@@ -5,11 +5,7 @@
 module DangerPackwerk
   #
   # PksOffense represents a violation from pks JSON output.
-  # It is designed to have a compatible interface with BasicReferenceOffense
-  # so it can be used with the Update::OffensesFormatter.
-  #
-  # It also provides adapter objects (reference, location) for compatibility
-  # with Packwerk::ReferenceOffense interface used by Check::OffensesFormatter.
+  # It has a compatible interface with BasicReferenceOffense for use with formatters.
   #
   class PksOffense < T::Struct
     extend T::Sig
@@ -24,50 +20,6 @@ class PksOffense < T::Struct
     const :strict, T::Boolean
     const :message, String
 
-    #
-    # Adapter classes for Packwerk::ReferenceOffense compatibility
-    # These allow PksOffense to be used with Check::OffensesFormatter
-    #
-    class PackageAdapter < T::Struct
-      const :name, String
-    end
-
-    class ConstantAdapter < T::Struct
-      const :name, String
-      const :location, String
-      const :package, PackageAdapter
-    end
-
-    class ReferenceAdapter < T::Struct
-      const :relative_path, String
-      const :constant, ConstantAdapter
-    end
-
-    class LocationAdapter < T::Struct
-      const :line, Integer
-    end
-
-    # Adapter for Packwerk::ReferenceOffense.reference
-    sig { returns(ReferenceAdapter) }
-    def reference
-      package_adapter = PackageAdapter.new(name: defining_pack_name)
-      constant_adapter = ConstantAdapter.new(
-        name: constant_name,
-        location: file, # Best approximation - pks doesn't provide constant definition location
-        package: package_adapter
-      )
-      ReferenceAdapter.new(
-        relative_path: file,
-        constant: constant_adapter
-      )
-    end
-
-    # Adapter for Packwerk::ReferenceOffense.location
-    sig { returns(LocationAdapter) }
-    def location
-      LocationAdapter.new(line: line)
-    end
-
     # Alias methods for compatibility with BasicReferenceOffense interface
     sig { returns(String) }
     def class_name

spec/danger_packwerk/danger_packwerk_spec.rb

@@ -57,10 +57,11 @@ module DangerPackwerk
             include Check::OffensesFormatter
 
             def format_offenses(offenses, repo_link, org_name, repo_url_builder:)
-              constant_location = offenses.first.reference.constant.location
-              constant_name = offenses.first.reference.constant.name
+              offense = offenses.first
+              constant_location = offense.file
+              constant_name = offense.constant_name
               url = repo_url_builder&.call(constant_location.to_s)
-              offenses.map { |offense| "#{offense.message} [#{constant_name}](#{url})" }.join("\n\n")
+              offenses.map { |o| "#{o.message} [#{constant_name}](#{url})" }.join("\n\n")
             end
           end
         end
@@ -601,7 +602,7 @@ def format_offenses(offenses, repo_link, org_name, repo_url_builder:)
                 actual_markdowns = dangerfile.status_report[:markdowns]
                 expect(actual_markdowns.count).to eq 1
                 actual_markdown = actual_markdowns.first
-                # The formatter uses offenses.first.reference.constant.location for all URLs
+                # The formatter uses offense.file for all URLs
                 expect(actual_markdown.message).to eq(<<~MSG.chomp)
                   Vanilla message about privacy violations [::PrivateConstant](https://github.com/MyOrg/my_repo/blob/my_branch/packs/referencing_pack/some_file.rb)