Merge pull request #272 from Shopify/disallow_update_with_args

Don't allow update on specific files

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

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|

lib/packwerk/parse_run.rb

@@ -16,13 +16,15 @@ 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
     end
     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)

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])
 

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
 

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)