Skip to content

Commit

Permalink
Fix check command stale todos with supplied files
Browse files Browse the repository at this point in the history 
Because the PackageTodo class didn't know what files had been
supplied it would erroneously suspect files had been deleted
if you only passed a single file on the command line.

We do this in a pre-commit hook with `bundle exec packwerk check $(git diff --name-only)`
and it was causing issues; the tool would assert there were stale
todos when there are not.

This changes the method signature of OffenseFormatter.show_stale_violations to
accept a FilesForProcessing instance instead of Set[String] and
plubms the FilesForProcessing through to PackageTodo#stale_violations? so
that it can call #files_specified? on it. If there were files specified
in the command invokation it skips the #deleted_files_for call.

Happy to make any adjustments. This addresses #369
  • Loading branch information
@jmcfarland-figma
jmcfarland-figma committed Oct 18, 2024
1 parent dc0b942 commit 98ccee7
Show file tree
Hide file tree
Showing 9 changed files with 171 additions and 118 deletions.
180 changes: 100 additions & 80 deletions Gemfile.lock
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,157 +16,177 @@ PATH
GEM
remote: https://rubygems.org/
specs:
actionpack (7.0.8.4)
actionview (= 7.0.8.4)
activesupport (= 7.0.8.4)
rack (~> 2.0, >= 2.2.4)
actionpack (7.2.1)
actionview (= 7.2.1)
activesupport (= 7.2.1)
nokogiri (>= 1.8.5)
racc
rack (>= 2.2.4, < 3.2)
rack-session (>= 1.0.1)
rack-test (>= 0.6.3)
rails-dom-testing (~> 2.0)
rails-html-sanitizer (~> 1.0, >= 1.2.0)
actionview (7.0.8.4)
activesupport (= 7.0.8.4)
rails-dom-testing (~> 2.2)
rails-html-sanitizer (~> 1.6)
useragent (~> 0.16)
actionview (7.2.1)
activesupport (= 7.2.1)
builder (~> 3.1)
erubi (~> 1.4)
rails-dom-testing (~> 2.0)
rails-html-sanitizer (~> 1.1, >= 1.2.0)
activesupport (7.0.8.4)
concurrent-ruby (~> 1.0, >= 1.0.2)
erubi (~> 1.11)
rails-dom-testing (~> 2.2)
rails-html-sanitizer (~> 1.6)
activesupport (7.2.1)
base64
bigdecimal
concurrent-ruby (~> 1.0, >= 1.3.1)
connection_pool (>= 2.2.5)
drb
i18n (>= 1.6, < 2)
logger (>= 1.4.2)
minitest (>= 5.1)
tzinfo (~> 2.0)
securerandom (>= 0.3)
tzinfo (~> 2.0, >= 2.0.5)
ast (2.4.2)
base64 (0.2.0)
better_html (2.1.1)
actionview (>= 6.0)
activesupport (>= 6.0)
ast (~> 2.0)
erubi (~> 1.4)
parser (>= 2.4)
smart_properties
bigdecimal (3.1.8)
builder (3.3.0)
byebug (11.1.3)
concurrent-ruby (1.3.4)
connection_pool (2.4.1)
constant_resolver (0.2.0)
crass (1.0.6)
drb (2.2.1)
erubi (1.13.0)
i18n (1.14.5)
i18n (1.14.6)
concurrent-ruby (~> 1.0)
io-console (0.7.2)
irb (1.14.1)
rdoc (>= 4.0.0)
reline (>= 0.4.2)
json (2.7.2)
language_server-protocol (3.17.0.3)
logger (1.6.1)
loofah (2.22.0)
crass (~> 1.0.2)
nokogiri (>= 1.12.0)
m (1.6.0)
m (1.6.2)
method_source (>= 0.6.7)
rake (>= 0.9.2.2)
method_source (1.1.0)
minitest (5.25.1)
minitest-focus (1.3.1)
minitest-focus (1.4.0)
minitest (>= 4, < 6)
mocha (1.14.0)
mocha (2.4.5)
ruby2_keywords (>= 0.0.5)
netrc (0.11.0)
nokogiri (1.16.7-aarch64-linux)
racc (~> 1.4)
nokogiri (1.16.7-arm64-darwin)
racc (~> 1.4)
nokogiri (1.16.7-x86_64-darwin)
racc (~> 1.4)
nokogiri (1.16.7-x86_64-linux)
racc (~> 1.4)
parallel (1.25.1)
parser (3.3.3.0)
parallel (1.26.3)
parser (3.3.5.0)
ast (~> 2.4.1)
racc
prism (0.27.0)
prism (1.2.0)
psych (5.1.2)
stringio
racc (1.8.1)
rack (2.2.9)
rack (3.1.7)
rack-session (2.0.0)
rack (>= 3.0.0)
rack-test (2.1.0)
rack (>= 1.3)
rackup (2.1.0)
rack (>= 3)
webrick (~> 1.8)
rails-dom-testing (2.2.0)
activesupport (>= 5.0.0)
minitest
nokogiri (>= 1.6)
rails-html-sanitizer (1.6.0)
loofah (~> 2.21)
nokogiri (~> 1.14)
railties (7.0.8.4)
actionpack (= 7.0.8.4)
activesupport (= 7.0.8.4)
method_source
railties (7.2.1)
actionpack (= 7.2.1)
activesupport (= 7.2.1)
irb (~> 1.13)
rackup (>= 1.0.0)
rake (>= 12.2)
thor (~> 1.0)
zeitwerk (~> 2.5)
thor (~> 1.0, >= 1.2.2)
zeitwerk (~> 2.6)
rainbow (3.1.1)
rake (13.0.6)
rbi (0.1.12)
prism (>= 0.18.0, < 0.28)
rake (13.2.1)
rbi (0.2.1)
prism (~> 1.0)
sorbet-runtime (>= 0.5.9204)
rdoc (6.7.0)
psych (>= 4.0.0)
regexp_parser (2.9.2)
rexml (3.3.6)
strscan
rubocop (1.64.1)
reline (0.5.10)
io-console (~> 0.5)
rubocop (1.66.1)
json (~> 2.3)
language_server-protocol (>= 3.17.0)
parallel (~> 1.10)
parser (>= 3.3.0.2)
rainbow (>= 2.2.2, < 4.0)
regexp_parser (>= 1.8, < 3.0)
rexml (>= 3.2.5, < 4.0)
rubocop-ast (>= 1.31.1, < 2.0)
regexp_parser (>= 2.4, < 3.0)
rubocop-ast (>= 1.32.2, < 2.0)
ruby-progressbar (~> 1.7)
unicode-display_width (>= 2.4.0, < 3.0)
rubocop-ast (1.31.3)
rubocop-ast (1.32.3)
parser (>= 3.3.1.0)
rubocop-performance (1.14.3)
rubocop (>= 1.7.0, < 2.0)
rubocop-ast (>= 0.4.0)
rubocop-shopify (2.9.0)
rubocop (~> 1.33)
rubocop-sorbet (0.6.11)
rubocop (>= 0.90.0)
rubocop-performance (1.22.1)
rubocop (>= 1.48.1, < 2.0)
rubocop-ast (>= 1.31.1, < 2.0)
rubocop-shopify (2.15.1)
rubocop (~> 1.51)
rubocop-sorbet (0.8.5)
rubocop (>= 1)
ruby-progressbar (1.13.0)
ruby2_keywords (0.0.5)
securerandom (0.3.1)
smart_properties (1.17.0)
sorbet (0.5.11367)
sorbet-static (= 0.5.11367)
sorbet-runtime (0.5.11367)
sorbet-static (0.5.11367-aarch64-linux)
sorbet-static (0.5.11367-universal-darwin)
sorbet-static (0.5.11367-x86_64-linux)
sorbet-static-and-runtime (0.5.11367)
sorbet (= 0.5.11367)
sorbet-runtime (= 0.5.11367)
spoom (1.3.2)
sorbet (0.5.11600)
sorbet-static (= 0.5.11600)
sorbet-runtime (0.5.11600)
sorbet-static (0.5.11600-universal-darwin)
sorbet-static-and-runtime (0.5.11600)
sorbet (= 0.5.11600)
sorbet-runtime (= 0.5.11600)
spoom (1.5.0)
erubi (>= 1.10.0)
prism (>= 0.19.0)
prism (>= 0.28.0)
sorbet-static-and-runtime (>= 0.5.10187)
thor (>= 0.19.2)
spring (4.0.0)
strscan (3.1.0)
tapioca (0.13.3)
spring (4.2.1)
stringio (3.1.1)
tapioca (0.16.3)
bundler (>= 2.2.25)
netrc (>= 0.11.0)
parallel (>= 1.21.0)
rbi (>= 0.1.4, < 0.2)
rbi (~> 0.2)
sorbet-static-and-runtime (>= 0.5.11087)
spoom (>= 1.2.0)
thor (>= 1.2.0)
yard-sorbet
thor (1.3.1)
thor (1.3.2)
tzinfo (2.0.6)
concurrent-ruby (~> 1.0)
unicode-display_width (2.5.0)
yard (0.9.36)
yard-sorbet (0.8.1)
sorbet-runtime (>= 0.5)
yard (>= 0.9)
zeitwerk (2.6.4)
unicode-display_width (2.6.0)
useragent (0.16.10)
webrick (1.8.2)
yard (0.9.37)
yard-sorbet (0.9.0)
sorbet-runtime
yard
zeitwerk (2.6.18)

PLATFORMS
aarch64-linux
arm64-darwin-21
arm64-darwin-22
x86_64-darwin
x86_64-darwin-20
x86_64-linux
arm64-darwin-23

DEPENDENCIES
byebug
Expand All @@ -186,4 +206,4 @@ DEPENDENCIES
zeitwerk

BUNDLED WITH
2.4.8
2.5.11
4 changes: 2 additions & 2 deletions lib/packwerk/commands/check_command.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,14 +35,14 @@ def run

messages = [
offenses_formatter.show_offenses(offense_collection.outstanding_offenses),
offenses_formatter.show_stale_violations(offense_collection, @files_for_processing.files),
offenses_formatter.show_stale_violations(offense_collection, @files_for_processing),
offenses_formatter.show_strict_mode_violations(unlisted_strict_mode_violations),
]

out.puts(messages.select(&:present?).join("\n") + "\n")

offense_collection.outstanding_offenses.empty? &&
!offense_collection.stale_violations?(@files_for_processing.files) &&
!offense_collection.stale_violations?(@files_for_processing) &&
unlisted_strict_mode_violations.empty?
end

Expand Down
6 changes: 3 additions & 3 deletions lib/packwerk/formatters/default_offenses_formatter.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@ def show_offenses(offenses)
EOS
end

sig { override.params(offense_collection: OffenseCollection, file_set: T::Set[String]).returns(String) }
def show_stale_violations(offense_collection, file_set)
if offense_collection.stale_violations?(file_set)
sig { override.params(offense_collection: OffenseCollection, files_for_processing: FilesForProcessing).returns(String) }
def show_stale_violations(offense_collection, files_for_processing)
if offense_collection.stale_violations?(files_for_processing)
"There were stale violations found, please run `packwerk update-todo`"
else
"No stale violations detected"
Expand Down
6 changes: 3 additions & 3 deletions lib/packwerk/offense_collection.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,10 @@ def add_offense(offense)
end
end

sig { params(for_files: T::Set[String]).returns(T::Boolean) }
def stale_violations?(for_files)
sig { params(files_for_processing: FilesForProcessing).returns(T::Boolean) }
def stale_violations?(files_for_processing)
@package_todos.values.any? do |package_todo|
package_todo.stale_violations?(for_files)
package_todo.stale_violations?(files_for_processing)
end
end

Expand Down
4 changes: 2 additions & 2 deletions lib/packwerk/offenses_formatter.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,8 @@ def formatter_by_identifier(name)
def show_offenses(offenses)
end

sig { abstract.params(offense_collection: OffenseCollection, for_files: T::Set[String]).returns(String) }
def show_stale_violations(offense_collection, for_files)
sig { abstract.params(offense_collection: OffenseCollection, files_for_processing: FilesForProcessing).returns(String) }
def show_stale_violations(offense_collection, files_for_processing)
end

sig { abstract.returns(String) }
Expand Down
10 changes: 7 additions & 3 deletions lib/packwerk/package_todo.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,16 @@ def add_entries(reference, violation_type)
listed?(reference, violation_type: violation_type)
end

sig { params(for_files: T::Set[String]).returns(T::Boolean) }
def stale_violations?(for_files)
sig { params(files_for_processing: FilesForProcessing).returns(T::Boolean) }
def stale_violations?(files_for_processing)
prepare_entries_for_dump

for_files = files_for_processing.files

old_entries.any? do |package, violations|
files = for_files + deleted_files_for(package)
# We don't try to detect deleted files when files were specified on the CLI because
# we don't know if the file was deleted or just not specified
files = files_for_processing.files_specified? ? for_files : for_files + deleted_files_for(package)
violations_for_files = package_violations_for(violations, files: files)

# We `next false` because if we cannot find existing violations for `for_files` within
Expand Down
12 changes: 12 additions & 0 deletions test/support/packwerk/factory_helper.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,5 +22,17 @@ def build_reference(
source_location: source_location,
)
end

def build_files_for_processing(
relative_file_paths: [],
configuration: Configuration.new(),
ignore_nested_packages: false
)
FilesForProcessing.new(
relative_file_paths,
configuration,
ignore_nested_packages
)
end
end
end
12 changes: 8 additions & 4 deletions test/unit/packwerk/offense_collection_test.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,25 +28,29 @@ class OffenseCollectionTest < Minitest::Test
test "#stale_violations? returns true if there are stale violations" do
@offense_collection.add_offense(@offense)
file_set = Set.new
FilesForProcessing.any_instance.stubs(:files).returns(file_set)
files_for_processing = build_files_for_processing

Packwerk::PackageTodo.any_instance
.expects(:stale_violations?)
.with(file_set)
.with(files_for_processing)
.returns(true)

assert @offense_collection.stale_violations?(file_set)
assert @offense_collection.stale_violations?(files_for_processing)
end

test "#stale_violations? returns false if no stale violations" do
@offense_collection.add_offense(@offense)
file_set = Set.new
FilesForProcessing.any_instance.stubs(:files).returns(file_set)
files_for_processing = build_files_for_processing

Packwerk::PackageTodo.any_instance
.expects(:stale_violations?)
.with(file_set)
.with(files_for_processing)
.returns(false)

refute @offense_collection.stale_violations?(Set.new)
refute @offense_collection.stale_violations?(files_for_processing)
end

test "#listed? returns true if constant is listed in file" do
Expand Down
Loading

0 comments on commit 98ccee7

Please sign in to comment.