From 114446596516fc4f92c2ffb839c28f4b186d26b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Mendon=C3=A7a=20Fran=C3=A7a?= Date: Wed, 8 Sep 2021 05:21:13 +0000 Subject: [PATCH] Packages with no violations should have their files removed --- lib/packwerk/offense_collection.rb | 26 ++++++++--- lib/packwerk/package_todo.rb | 7 ++- lib/packwerk/parse_run.rb | 16 ++++--- lib/packwerk/run_context.rb | 11 ++--- test/integration/offense_collection_test.rb | 9 +++- test/unit/parse_run_test.rb | 49 +++++++++++++++++++++ 6 files changed, 99 insertions(+), 19 deletions(-) diff --git a/lib/packwerk/offense_collection.rb b/lib/packwerk/offense_collection.rb index 17aaba9b7..16ff55c96 100644 --- a/lib/packwerk/offense_collection.rb +++ b/lib/packwerk/offense_collection.rb @@ -59,11 +59,10 @@ def stale_violations?(for_files) end end - sig { void } - def dump_package_todo_files - @package_todo.each do |_, package_todo_file| - package_todo_file.dump - end + sig { params(package_set: Packwerk::PackageSet).void } + def persist_package_todo_files(package_set) + dump_package_todo_files + cleanup_extra_package_todo_files(package_set) end sig { returns(T::Array[Packwerk::Offense]) } @@ -73,6 +72,23 @@ def outstanding_offenses private + sig { params(package_set: Packwerk::PackageSet).void } + def cleanup_extra_package_todo_files(package_set) + packages_without_todos = (package_set.packages.values - @package_todo.keys) + + packages_without_todos.each do |package| + Packwerk::PackageTodo.new( + package, + package_todo_file_for(package), + ).delete_if_exists + end + end + + sig { void } + def dump_package_todo_files + @package_todo.each_value(&:dump) + end + sig { params(package: Packwerk::Package).returns(Packwerk::PackageTodo) } def package_todo_for(package) @package_todo[package] ||= Packwerk::PackageTodo.new( diff --git a/lib/packwerk/package_todo.rb b/lib/packwerk/package_todo.rb index e21c64013..562be9a6b 100644 --- a/lib/packwerk/package_todo.rb +++ b/lib/packwerk/package_todo.rb @@ -92,7 +92,7 @@ def stale_violations?(for_files) sig { void } def dump if @new_entries.empty? - File.delete(@filepath) if File.exist?(@filepath) + delete_if_exists else prepare_entries_for_dump message = <<~MESSAGE @@ -110,6 +110,11 @@ def dump end end + sig { void } + def delete_if_exists + File.delete(@filepath) if File.exist?(@filepath) + end + private sig { returns(EntriesType) } diff --git a/lib/packwerk/parse_run.rb b/lib/packwerk/parse_run.rb index 1c3bfed7b..793c29540 100644 --- a/lib/packwerk/parse_run.rb +++ b/lib/packwerk/parse_run.rb @@ -34,7 +34,8 @@ def initialize( sig { returns(Result) } def detect_stale_violations - offense_collection = find_offenses + run_context = Packwerk::RunContext.from_configuration(@configuration) + offense_collection = find_offenses(run_context) result_status = !offense_collection.stale_violations?(@relative_file_set) message = @offenses_formatter.show_stale_violations(offense_collection, @relative_file_set) @@ -44,8 +45,9 @@ def detect_stale_violations sig { returns(Result) } def update_todo - offense_collection = find_offenses - offense_collection.dump_package_todo_files + run_context = Packwerk::RunContext.from_configuration(@configuration) + offense_collection = find_offenses(run_context) + offense_collection.persist_package_todo_files(run_context.package_set) message = <<~EOS #{@offenses_formatter.show_offenses(offense_collection.errors)} @@ -57,7 +59,8 @@ def update_todo sig { returns(Result) } def check - offense_collection = find_offenses(show_errors: true) + run_context = Packwerk::RunContext.from_configuration(@configuration) + offense_collection = find_offenses(run_context, show_errors: true) messages = [ @offenses_formatter.show_offenses(offense_collection.outstanding_offenses), @@ -72,12 +75,11 @@ def check private - sig { params(show_errors: T::Boolean).returns(OffenseCollection) } - def find_offenses(show_errors: false) + sig { params(run_context: Packwerk::RunContext, show_errors: T::Boolean).returns(OffenseCollection) } + def find_offenses(run_context, show_errors: false) offense_collection = OffenseCollection.new(@configuration.root_path) @progress_formatter.started(@relative_file_set) - run_context = Packwerk::RunContext.from_configuration(@configuration) all_offenses = T.let([], T::Array[Offense]) process_file = T.let(->(relative_file) do diff --git a/lib/packwerk/run_context.rb b/lib/packwerk/run_context.rb index 43a3b088c..e76bdb56e 100644 --- a/lib/packwerk/run_context.rb +++ b/lib/packwerk/run_context.rb @@ -71,6 +71,7 @@ def initialize( @file_processor = T.let(nil, T.nilable(FileProcessor)) @context_provider = T.let(nil, T.nilable(ConstantDiscovery)) + @package_set = T.let(nil, T.nilable(PackageSet)) # We need to initialize this before we fork the process, see https://github.com/Shopify/packwerk/issues/182 @cache = T.let( Cache.new(enable_cache: @cache_enabled, cache_directory: @cache_directory, config_path: @config_path), Cache @@ -90,6 +91,11 @@ def process_file(relative_file:) processed_file.offenses + references.flat_map { |reference| reference_checker.call(reference) } end + sig { returns(PackageSet) } + def package_set + @package_set ||= ::Packwerk::PackageSet.load_all_from(@root_path, package_pathspec: @package_paths) + end + private sig { returns(FileProcessor) } @@ -123,11 +129,6 @@ def resolver ) end - sig { returns(PackageSet) } - def package_set - ::Packwerk::PackageSet.load_all_from(@root_path, package_pathspec: @package_paths) - end - sig { returns(T::Array[ConstantNameInspector]) } def constant_name_inspectors [ diff --git a/test/integration/offense_collection_test.rb b/test/integration/offense_collection_test.rb index da1690590..03827273e 100644 --- a/test/integration/offense_collection_test.rb +++ b/test/integration/offense_collection_test.rb @@ -37,9 +37,10 @@ class OffenseCollectionTest < Minitest::Test message: "some message", violation_type: ReferenceChecking::Checkers::DependencyChecker::VIOLATION_TYPE ) + @offense_collection.add_offense(offense1) @offense_collection.add_offense(offense2) - @offense_collection.dump_package_todo_files + dump_package_todo_files expected = { "components/destination" => { @@ -49,6 +50,12 @@ class OffenseCollectionTest < Minitest::Test } assert_equal expected, YAML.load_file("package_todo.yml") end + + private + + def dump_package_todo_files + @offense_collection.persist_package_todo_files(PackageSet.new([])) + end end end end diff --git a/test/unit/parse_run_test.rb b/test/unit/parse_run_test.rb index 4d2e5b6bf..74626ec4b 100644 --- a/test/unit/parse_run_test.rb +++ b/test/unit/parse_run_test.rb @@ -73,6 +73,55 @@ class ParseRunTest < Minitest::Test refute result.status end + test "#update_todo cleans up old package_todo files" do + use_template(:minimal) + + FileUtils.mkdir_p("app/models") + File.write("app/models/my_model.rb", <<~YML.strip) + class MyModel + def something + Order + end + end + YML + + File.write("package_todo.yml", <<~YML.strip) + --- + "components/sales": + "::Order": + violations: + - privacy + files: + - app/models/my_model.rb + YML + + File.write("components/sales/package_todo.yml", <<~YML.strip) + --- + "components/destination": + "::SomeName": + violations: + - privacy + files: + - a/b/c.rb + YML + + parse_run = Packwerk::ParseRun.new( + relative_file_set: Set.new(["app/models/my_model.rb", "components/sales/app/models/order.rb"]), + configuration: Configuration.from_path + ) + result = parse_run.update_todo + + expected = <<~EOS + No offenses detected + ✅ `package_todo.yml` has been updated. + EOS + assert_equal expected, result.message + assert result.status + + assert File.exist?("package_todo.yml") + refute File.exist?("components/sales/package_todo.yml") + end + test "#check only reports error progress for unlisted violations" do use_template(:minimal) offense = ReferenceOffense.new(