From 3de199a4e286b9380780534cd41f1904c11a5276 Mon Sep 17 00:00:00 2001 From: Alex Evanczuk Date: Mon, 18 Apr 2022 09:04:06 -0400 Subject: [PATCH 1/3] Change behavior to leave comments on new files if the constant is already in `deprecated_references.yml` --- .../basic_reference_offense.rb | 46 ++++++++++++++++--- ...anger_deprecated_references_yml_changes.rb | 24 ++++++++-- lib/danger-packwerk/violation_diff.rb | 1 + ..._deprecated_references_yml_changes_spec.rb | 32 ++++++------- 4 files changed, 76 insertions(+), 27 deletions(-) diff --git a/lib/danger-packwerk/basic_reference_offense.rb b/lib/danger-packwerk/basic_reference_offense.rb index 8f5a8c7..6c7020c 100644 --- a/lib/danger-packwerk/basic_reference_offense.rb +++ b/lib/danger-packwerk/basic_reference_offense.rb @@ -38,38 +38,70 @@ def hash const :file, String const :to_package_name, String const :type, String - const :location, Location + const :class_name_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) + 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? + + 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 + class_name_location: class_name_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..ab0e27d 100644 --- a/lib/danger-packwerk/danger_deprecated_references_yml_changes.rb +++ b/lib/danger-packwerk/danger_deprecated_references_yml_changes.rb @@ -44,9 +44,21 @@ 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 + # 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 + markdown( added_offenses_formatter.call(violations), line: location.line_number, @@ -61,6 +73,7 @@ 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, @@ -71,20 +84,23 @@ 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) + 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 + removed_violations: removed_violations, + all_violations_before: all_violations_before ) end diff --git a/lib/danger-packwerk/violation_diff.rb b/lib/danger-packwerk/violation_diff.rb index 89404ea..19d74f3 100644 --- a/lib/danger-packwerk/violation_diff.rb +++ b/lib/danger-packwerk/violation_diff.rb @@ -9,5 +9,6 @@ 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 6908cab..656ac5a 100644 --- a/spec/danger_packwerk/danger_deprecated_references_yml_changes_spec.rb +++ b/spec/danger_packwerk/danger_deprecated_references_yml_changes_spec.rb @@ -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 From 9078ba9a0c12b8d2dd56386f519e64b9680a6090 Mon Sep 17 00:00:00 2001 From: Alex Evanczuk Date: Mon, 18 Apr 2022 09:09:33 -0400 Subject: [PATCH 2/3] switch to simpler implementation --- .../basic_reference_offense.rb | 6 +---- ...anger_deprecated_references_yml_changes.rb | 19 +++----------- lib/danger-packwerk/violation_diff.rb | 1 - ..._deprecated_references_yml_changes_spec.rb | 26 +++++++++---------- 4 files changed, 17 insertions(+), 35 deletions(-) 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 From 2f168b613e657513a4857e5048384a76d839aef2 Mon Sep 17 00:00:00 2001 From: Alex Evanczuk Date: Tue, 19 Apr 2022 15:39:48 -0400 Subject: [PATCH 3/3] Bump version --- Gemfile.lock | 10 +++++----- lib/danger-packwerk/version.rb | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) 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/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