Skip to content

Simplify offense data model by removing adapter pattern #63

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
martinemde merged 1 commit into from
Jan 9, 2026
Merged

Simplify offense data model by removing adapter pattern #63

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions lib/danger-packwerk/check/default_formatter.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
2 changes: 1 addition & 1 deletion lib/danger-packwerk/check/offenses_formatter.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
23 changes: 10 additions & 13 deletions lib/danger-packwerk/danger_packwerk.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.'
Expand Down Expand Up @@ -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
Expand All @@ -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)
Expand All @@ -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

Expand Down
50 changes: 1 addition & 49 deletions lib/danger-packwerk/pks_offense.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
9 changes: 5 additions & 4 deletions spec/danger_packwerk/danger_packwerk_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)

Expand Down