diff --git a/lib/danger-packwerk/basic_reference_offense.rb b/lib/danger-packwerk/basic_reference_offense.rb index 6c7020c..b824a0c 100644 --- a/lib/danger-packwerk/basic_reference_offense.rb +++ b/lib/danger-packwerk/basic_reference_offense.rb @@ -38,7 +38,6 @@ 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]) } @@ -85,14 +84,12 @@ 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( @@ -100,7 +97,6 @@ def self.from(deprecated_references_yml) file: file, to_package_name: violation.to_package_name, type: violation.type, - class_name_location: class_name_location, file_location: file_location ) end diff --git a/lib/danger-packwerk/danger_deprecated_references_yml_changes.rb b/lib/danger-packwerk/danger_deprecated_references_yml_changes.rb index ab0e27d..fb600a1 100644 --- a/lib/danger-packwerk/danger_deprecated_references_yml_changes.rb +++ b/lib/danger-packwerk/danger_deprecated_references_yml_changes.rb @@ -45,19 +45,10 @@ 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 - # 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 + location = T.must(violations.first).file_location markdown( added_offenses_formatter.call(violations), @@ -73,7 +64,6 @@ 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, @@ -85,22 +75,19 @@ 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, - all_violations_before: all_violations_before + removed_violations: removed_violations ) end diff --git a/lib/danger-packwerk/violation_diff.rb b/lib/danger-packwerk/violation_diff.rb index 19d74f3..89404ea 100644 --- a/lib/danger-packwerk/violation_diff.rb +++ b/lib/danger-packwerk/violation_diff.rb @@ -9,6 +9,5 @@ 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 diff --git a/spec/danger_packwerk/danger_deprecated_references_yml_changes_spec.rb b/spec/danger_packwerk/danger_deprecated_references_yml_changes_spec.rb index 656ac5a..2dfcad5 100644 --- a/spec/danger_packwerk/danger_deprecated_references_yml_changes_spec.rb +++ b/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