From 9307734451c5871664b8a2b34c17549fc9003acb Mon Sep 17 00:00:00 2001 From: Gannon McGibbon Date: Fri, 4 Nov 2022 20:16:20 -0500 Subject: [PATCH] Don't allow update on specific files When updating the todo, Packwerk should know about all packages in an application. Most of the time, running update-todo with arguments doesn't make sense and is prone to errors. So, let's stop this usage and make sure developers are always updating todos without arguments. --- lib/packwerk/cli.rb | 32 +++++++++++++---------- lib/packwerk/files_for_processing.rb | 35 +++++++++++++++++--------- lib/packwerk/parse_run.rb | 11 ++++++++ test/unit/cli_test.rb | 25 +++++++++++++----- test/unit/files_for_processing_test.rb | 24 +++++++++++------- test/unit/parse_run_test.rb | 15 +++++++++++ 6 files changed, 102 insertions(+), 40 deletions(-) diff --git a/lib/packwerk/cli.rb b/lib/packwerk/cli.rb index f560a1e11..1cd4fcf94 100644 --- a/lib/packwerk/cli.rb +++ b/lib/packwerk/cli.rb @@ -130,17 +130,20 @@ def output_result(result) params( relative_file_paths: T::Array[String], ignore_nested_packages: T::Boolean - ).returns(FilesForProcessing::RelativeFileSet) + ).returns(FilesForProcessing) end def fetch_files_to_process(relative_file_paths, ignore_nested_packages) - relative_file_set = FilesForProcessing.fetch( + files_for_processing = FilesForProcessing.fetch( relative_file_paths: relative_file_paths, ignore_nested_packages: ignore_nested_packages, configuration: @configuration ) - abort("No files found or given. "\ - "Specify files or check the include and exclude glob in the config file.") if relative_file_set.empty? - relative_file_set + abort(<<~MSG.squish) if files_for_processing.files.empty? + No files found or given. + Specify files or check the include and exclude glob in the config file. + MSG + + files_for_processing end sig { params(_paths: T::Array[String]).returns(T::Boolean) } @@ -178,35 +181,38 @@ def list_validation_errors(result) end end - sig { params(params: T.untyped).returns(ParseRun) } - def parse_run(params) + sig { params(args: T::Array[String]).returns(ParseRun) } + def parse_run(args) relative_file_paths = T.let([], T::Array[String]) ignore_nested_packages = nil formatter = @offenses_formatter - if params.any? { |p| p.include?("--packages") } + if args.any? { |arg| arg.include?("--packages") } OptionParser.new do |parser| parser.on("--packages=PACKAGESLIST", Array, "package names, comma separated") do |p| relative_file_paths = p end - end.parse!(params) + end.parse!(args) ignore_nested_packages = true else - relative_file_paths = params + relative_file_paths = args ignore_nested_packages = false end - if params.any? { |p| p.include?("--offenses-formatter") } + if args.any? { |arg| arg.include?("--offenses-formatter") } OptionParser.new do |parser| parser.on("--offenses-formatter=FORMATTER", String, "identifier of offenses formatter to use") do |formatter_identifier| formatter = OffensesFormatter.find(formatter_identifier) end - end.parse!(params) + end.parse!(args) end + files_for_processing = fetch_files_to_process(relative_file_paths, ignore_nested_packages) + ParseRun.new( - relative_file_set: fetch_files_to_process(relative_file_paths, ignore_nested_packages), + relative_file_set: files_for_processing.files, + file_set_specified: files_for_processing.files_specified?, configuration: @configuration, progress_formatter: @progress_formatter, offenses_formatter: formatter diff --git a/lib/packwerk/files_for_processing.rb b/lib/packwerk/files_for_processing.rb index efc2ca9c7..034f2207a 100644 --- a/lib/packwerk/files_for_processing.rb +++ b/lib/packwerk/files_for_processing.rb @@ -15,10 +15,10 @@ class << self relative_file_paths: T::Array[String], configuration: Configuration, ignore_nested_packages: T::Boolean - ).returns(RelativeFileSet) + ).returns(FilesForProcessing) end def fetch(relative_file_paths:, configuration:, ignore_nested_packages: false) - new(relative_file_paths, configuration, ignore_nested_packages).files + new(relative_file_paths, configuration, ignore_nested_packages) end end @@ -33,37 +33,48 @@ def initialize(relative_file_paths, configuration, ignore_nested_packages) @relative_file_paths = relative_file_paths @configuration = configuration @ignore_nested_packages = ignore_nested_packages - @custom_files = T.let(nil, T.nilable(RelativeFileSet)) + @specified_files = T.let(nil, T.nilable(RelativeFileSet)) + @files = T.let(nil, T.nilable(RelativeFileSet)) end sig { returns(RelativeFileSet) } def files - include_files = if custom_files.empty? + @files ||= files_for_processing + end + + sig { returns(T::Boolean) } + def files_specified? + specified_files.any? + end + + private + + sig { returns(RelativeFileSet) } + def files_for_processing + all_included_files = if specified_files.empty? configured_included_files else - configured_included_files & custom_files + configured_included_files & specified_files end - include_files - configured_excluded_files + all_included_files - configured_excluded_files end - private - sig { returns(RelativeFileSet) } - def custom_files - @custom_files ||= Set.new( + def specified_files + @specified_files ||= Set.new( @relative_file_paths.map do |relative_file_path| if File.file?(relative_file_path) relative_file_path else - custom_included_files(relative_file_path) + specified_included_files(relative_file_path) end end ).flatten end sig { params(relative_file_path: String).returns(RelativeFileSet) } - def custom_included_files(relative_file_path) + def specified_included_files(relative_file_path) # Note, assuming include globs are always relative paths relative_includes = @configuration.include relative_files = Dir.glob([File.join(relative_file_path, "**", "*")]).select do |relative_path| diff --git a/lib/packwerk/parse_run.rb b/lib/packwerk/parse_run.rb index c5cf558fc..3ad768c3b 100644 --- a/lib/packwerk/parse_run.rb +++ b/lib/packwerk/parse_run.rb @@ -16,6 +16,7 @@ class ParseRun params( relative_file_set: FilesForProcessing::RelativeFileSet, configuration: Configuration, + file_set_specified: T::Boolean, offenses_formatter: T.nilable(OffensesFormatter), progress_formatter: Formatters::ProgressFormatter, ).void @@ -23,6 +24,7 @@ class ParseRun def initialize( relative_file_set:, configuration:, + file_set_specified: false, offenses_formatter: nil, progress_formatter: Formatters::ProgressFormatter.new(StringIO.new) ) @@ -31,10 +33,19 @@ def initialize( @progress_formatter = progress_formatter @offenses_formatter = T.let(offenses_formatter || configuration.offenses_formatter, Packwerk::OffensesFormatter) @relative_file_set = relative_file_set + @file_set_specified = file_set_specified end sig { returns(Result) } def update_todo + if @file_set_specified + message = <<~MSG.squish + ⚠️ update-todo must be called without any file arguments. + MSG + + return Result.new(message: message, status: false) + end + run_context = Packwerk::RunContext.from_configuration(@configuration) offense_collection = find_offenses(run_context) offense_collection.persist_package_todo_files(run_context.package_set) diff --git a/test/unit/cli_test.rb b/test/unit/cli_test.rb index 6d8545b94..1a8188dfd 100644 --- a/test/unit/cli_test.rb +++ b/test/unit/cli_test.rb @@ -21,6 +21,7 @@ class CliTest < Minitest::Test test "#execute_command with the subcommand check starts processing files" do use_template(:blank) + file_path = "path/of/exile.rb" violation_message = "This is a violation of code health." offense = Offense.new(file: file_path, message: violation_message) @@ -34,7 +35,9 @@ class CliTest < Minitest::Test cli = ::Packwerk::Cli.new(out: string_io, configuration: configuration) # TODO: Dependency injection for a "target finder" (https://github.com/Shopify/packwerk/issues/164) - ::Packwerk::FilesForProcessing.stubs(fetch: Set.new([file_path])) + FilesForProcessing.any_instance.stubs( + files: Set.new([file_path]) + ) success = cli.execute_command(["check", file_path]) @@ -46,6 +49,7 @@ class CliTest < Minitest::Test test "#execute_command with the subcommand check traps the interrupt signal" do use_template(:blank) + file_path = "path/of/exile.rb" interrupt_message = "Manually interrupted. Violations caught so far are listed below:" violation_message = "This is a violation of code health." @@ -64,7 +68,9 @@ class CliTest < Minitest::Test cli = ::Packwerk::Cli.new(out: string_io, configuration: configuration) - ::Packwerk::FilesForProcessing.stubs(fetch: Set.new([file_path, "test.rb", "foo.rb"])) + FilesForProcessing.any_instance.stubs( + files: Set.new([file_path, "test.rb", "foo.rb"]) + ) success = cli.execute_command(["check", file_path]) @@ -85,6 +91,7 @@ class CliTest < Minitest::Test test "#execute_command with validate subcommand runs application validator and succeeds if no errors" do use_template(:blank) + string_io = StringIO.new cli = ::Packwerk::Cli.new(out: string_io) @@ -129,6 +136,7 @@ class CliTest < Minitest::Test test "#execute_command using a custom offenses class" do use_template(:blank) + offenses_formatter = Class.new do include Packwerk::OffensesFormatter @@ -144,7 +152,6 @@ def identifier "custom" end end - file_path = "path/of/exile.rb" violation_message = "This is a violation of code health." offense = Offense.new(file: file_path, message: violation_message) @@ -162,7 +169,9 @@ def identifier offenses_formatter: T.unsafe(offenses_formatter).new ) - ::Packwerk::FilesForProcessing.stubs(fetch: Set.new([file_path])) + FilesForProcessing.any_instance.stubs( + files: Set.new([file_path]) + ) success = cli.execute_command(["check", file_path]) @@ -196,7 +205,9 @@ def identifier cli = ::Packwerk::Cli.new(out: string_io) end - ::Packwerk::FilesForProcessing.stubs(fetch: Set.new([file_path])) + FilesForProcessing.any_instance.stubs( + files: Set.new([file_path]) + ) success = T.must(cli).execute_command(["check", file_path]) @@ -232,7 +243,9 @@ def identifier cli = ::Packwerk::Cli.new(out: string_io) end - ::Packwerk::FilesForProcessing.stubs(fetch: Set.new([file_path])) + FilesForProcessing.any_instance.stubs( + files: Set.new([file_path]) + ) success = T.must(cli).execute_command(["check", "--offenses-formatter=default", file_path]) diff --git a/test/unit/files_for_processing_test.rb b/test/unit/files_for_processing_test.rb index 1e4eea8b8..f21f4df48 100644 --- a/test/unit/files_for_processing_test.rb +++ b/test/unit/files_for_processing_test.rb @@ -19,33 +19,39 @@ class FilesForProcessingTest < Minitest::Test end test "fetch with custom paths includes only include glob in custom paths" do - files = ::Packwerk::FilesForProcessing.fetch(relative_file_paths: [@package_path], configuration: @configuration) + files = ::Packwerk::FilesForProcessing.fetch( + relative_file_paths: [@package_path], + configuration: @configuration, + ).files included_file_pattern = File.join(@package_path, "**/*.rb") assert_all_match(files, [included_file_pattern]) end test "fetch with custom paths excludes the exclude glob in custom paths" do - files = ::Packwerk::FilesForProcessing.fetch(relative_file_paths: [@package_path], configuration: @configuration) + files = ::Packwerk::FilesForProcessing.fetch( + relative_file_paths: [@package_path], + configuration: @configuration + ).files excluded_file_pattern = File.join(@configuration.root_path, @package_path, "**/temp.rb") refute_any_match(files, [excluded_file_pattern]) end test "fetch with no custom paths includes only include glob across codebase" do - files = ::Packwerk::FilesForProcessing.fetch(relative_file_paths: [], configuration: @configuration) + files = ::Packwerk::FilesForProcessing.fetch(relative_file_paths: [], configuration: @configuration).files assert_all_match(files, @configuration.include) end test "fetch with no custom paths excludes the exclude glob across codebase" do - files = ::Packwerk::FilesForProcessing.fetch(relative_file_paths: [], configuration: @configuration) + files = ::Packwerk::FilesForProcessing.fetch(relative_file_paths: [], configuration: @configuration).files excluded_file_patterns = @configuration.exclude.map { |pattern| File.join(@configuration.root_path, pattern) } refute_any_match(files, Set.new(excluded_file_patterns)) end test "fetch does not return duplicated file paths" do - files = ::Packwerk::FilesForProcessing.fetch(relative_file_paths: [], configuration: @configuration) + files = ::Packwerk::FilesForProcessing.fetch(relative_file_paths: [], configuration: @configuration).files assert_equal files, Set.new(files) end @@ -54,7 +60,7 @@ class FilesForProcessingTest < Minitest::Test relative_file_paths: ["."], configuration: @configuration, ignore_nested_packages: false - ) + ).files assert_all_match(files, Set.new(@configuration.include)) end @@ -64,7 +70,7 @@ class FilesForProcessingTest < Minitest::Test relative_file_paths: [], configuration: @configuration, ignore_nested_packages: true - ) + ).files assert_all_match(files, @configuration.include) end @@ -74,7 +80,7 @@ class FilesForProcessingTest < Minitest::Test relative_file_paths: ["."], configuration: @configuration, ignore_nested_packages: true - ) + ).files refute_any_match(files, Set.new([File.join(@configuration.root_path, "components/sales", "**/*.rb")])) refute_any_match(files, Set.new([File.join(@configuration.root_path, "components/timeline", "**/*.rb")])) @@ -87,7 +93,7 @@ class FilesForProcessingTest < Minitest::Test "components/sales/app/views/order.html.erb", ], configuration: @configuration - ) + ).files included_file_patterns = @configuration.include diff --git a/test/unit/parse_run_test.rb b/test/unit/parse_run_test.rb index 89345631d..d0f42fe5c 100644 --- a/test/unit/parse_run_test.rb +++ b/test/unit/parse_run_test.rb @@ -59,6 +59,21 @@ class ParseRunTest < Minitest::Test refute result.status end + test "#update-todo returns exit code 1 when ran with file args" do + use_template(:minimal) + + parse_run = Packwerk::ParseRun.new( + relative_file_set: Set.new(["path/of/exile.rb"]), + file_set_specified: true, + configuration: Configuration.from_path + ) + result = parse_run.update_todo + + expected = "⚠️ update-todo must be called without any file arguments." + assert_equal expected, result.message + refute result.status + end + test "#update_todo cleans up old package_todo files" do use_template(:minimal)