diff --git a/Gemfile.lock b/Gemfile.lock index 19047e3..cb6a5e1 100644 --- a/Gemfile.lock +++ b/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) diff --git a/lib/danger-packwerk/basic_reference_offense.rb b/lib/danger-packwerk/basic_reference_offense.rb index 8f5a8c7..b824a0c 100644 --- a/lib/danger-packwerk/basic_reference_offense.rb +++ b/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 diff --git a/lib/danger-packwerk/danger_deprecated_references_yml_changes.rb b/lib/danger-packwerk/danger_deprecated_references_yml_changes.rb index d6e6b1c..fb600a1 100644 --- a/lib/danger-packwerk/danger_deprecated_references_yml_changes.rb +++ b/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 diff --git a/lib/danger-packwerk/version.rb b/lib/danger-packwerk/version.rb index 624f96d..0bc6136 100644 --- a/lib/danger-packwerk/version.rb +++ b/lib/danger-packwerk/version.rb @@ -2,5 +2,5 @@ # frozen_string_literal: true module DangerPackwerk - VERSION = '0.1.0' + VERSION = '0.1.1' 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 6908cab..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 @@ -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