Merge pull request #89 from nevinera/fix-all-files-runs

Fix handling of changed-files configuration

CHANGELOG.md

@@ -1,5 +1,16 @@
 # Changelog
 
+## Release 1.2.1
+
+* Fix the handling of the various ways to specify whether tools should limit
+  their targets to changed files or run against the entire repository. The
+  configuration systems had disagreements on what to call the options in
+  question, which resulted in some configuration entries being ignored. We
+  enforce a set of validations on reads and writes now to avoid such a problem
+  in the future (#89, resolves #88)
+* Add coverage-checking, and then improve test coverage and remove unreferenced
+  code as a result. (#87)
+
 ## Release 1.2.0
 
 * Support `--light`, `--quiet`, and `--logging LEVEL` arguments for less output

lib/quiet_quality/cli/arg_parser.rb

@@ -45,12 +45,12 @@ def validated_tool_names(names)
       # options; if they don't, they are global options. (optparse allows an optional argument
       # to a flag if the string representing it is not a 'string in all caps'. So `[FOO]` or `foo`
       # would be optional, but `FOO` would be required. This helper simplifies handling those.
-      def read_tool_or_global_option(name, tool, value)
+      def read_tool_or_global_option(name:, into:, tool:, value:)
         if tool
           validate_value_from("tool", tool, Tools::AVAILABLE)
-          set_tool_option(tool, name, value)
+          set_tool_option(tool, into, value)
         else
-          set_global_option(name, value)
+          set_global_option(into, value)
         end
       end
 
@@ -114,11 +114,11 @@ def setup_annotation_options(parser)
 
       def setup_file_target_options(parser)
         parser.on("-a", "--all-files [tool]", "Use the tool(s) on all files") do |tool|
-          read_tool_or_global_option(:all_files, tool, true)
+          read_tool_or_global_option(name: :all_files, into: :limit_targets, tool: tool, value: false)
         end
 
         parser.on("-c", "--changed-files [tool]", "Use the tool(s) only on changed files") do |tool|
-          read_tool_or_global_option(:all_files, tool, false)
+          read_tool_or_global_option(name: :all_files, into: :limit_targets, tool: tool, value: true)
         end
 
         parser.on("-B", "--comparison-branch BRANCH", "Specify the branch to compare against") do |branch|
@@ -128,11 +128,11 @@ def setup_file_target_options(parser)
 
       def setup_filter_messages_options(parser)
         parser.on("-f", "--filter-messages [tool]", "Filter messages from tool(s) based on changed lines") do |tool|
-          read_tool_or_global_option(:filter_messages, tool, true)
+          read_tool_or_global_option(name: :filter_messages, into: :filter_messages, tool: tool, value: true)
         end
 
         parser.on("-u", "--unfiltered [tool]", "Don't filter messages from tool(s)") do |tool|
-          read_tool_or_global_option(:filter_messages, tool, false)
+          read_tool_or_global_option(name: :filter_messages, into: :filter_messages, tool: tool, value: false)
         end
       end
 

lib/quiet_quality/config/parsed_options.rb

@@ -1,6 +1,26 @@
 module QuietQuality
   module Config
     class ParsedOptions
+      InvalidOptionName = Class.new(Error)
+
+      GLOBAL_OPTIONS = [
+        :no_config,
+        :config_path,
+        :annotator,
+        :executor,
+        :comparison_branch,
+        :logging,
+        :limit_targets,
+        :filter_messages,
+        :file_filter
+      ].to_set
+
+      TOOL_OPTIONS = [
+        :limit_targets,
+        :filter_messages,
+        :file_filter
+      ].to_set
+
       def initialize
         @tools = []
         @tool_options = {}
@@ -20,21 +40,37 @@ def printing_version?
       end
 
       def set_global_option(name, value)
+        validate_global_option(name)
         @global_options[name.to_sym] = value
       end
 
       def global_option(name)
+        validate_global_option(name)
         @global_options.fetch(name.to_sym, nil)
       end
 
       def set_tool_option(tool, name, value)
+        validate_tool_option(name)
         @tool_options[tool.to_sym] ||= {}
         @tool_options[tool.to_sym][name.to_sym] = value
       end
 
       def tool_option(tool, name)
+        validate_tool_option(name)
         @tool_options.dig(tool.to_sym, name.to_sym)
       end
+
+      private
+
+      def validate_global_option(name)
+        return if GLOBAL_OPTIONS.include?(name.to_sym)
+        fail(InvalidOptionName, "Option name #{name} is not a recognized global ParsedOption")
+      end
+
+      def validate_tool_option(name)
+        return if TOOL_OPTIONS.include?(name.to_sym)
+        fail(InvalidOptionName, "Option name #{name} is not a recognized tool ParsedOption")
+      end
     end
   end
 end

lib/quiet_quality/config/parser.rb

@@ -42,8 +42,8 @@ def store_global_options(opts)
         read_global_option(opts, :annotator, :annotator, as: :symbol, validate_from: Annotators::ANNOTATOR_TYPES)
         read_global_option(opts, :annotate, :annotator, as: :symbol, validate_from: Annotators::ANNOTATOR_TYPES)
         read_global_option(opts, :comparison_branch, :comparison_branch, as: :string)
-        read_global_option(opts, :changed_files, :changed_files, as: :boolean)
-        read_global_option(opts, :all_files, :changed_files, as: :reversed_boolean)
+        read_global_option(opts, :changed_files, :limit_targets, as: :boolean)
+        read_global_option(opts, :all_files, :limit_targets, as: :reversed_boolean)
         read_global_option(opts, :filter_messages, :filter_messages, as: :boolean)
         read_global_option(opts, :unfiltered, :filter_messages, as: :reversed_boolean)
         read_global_option(opts, :logging, :logging, as: :symbol, validate_from: Logging::LEVELS)
@@ -61,8 +61,8 @@ def store_tool_options_for(opts, tool_name)
 
         read_tool_option(opts, tool_name, :filter_messages, :filter_messages, as: :boolean)
         read_tool_option(opts, tool_name, :unfiltered, :filter_messages, as: :reversed_boolean)
-        read_tool_option(opts, tool_name, :changed_files, :changed_files, as: :boolean)
-        read_tool_option(opts, tool_name, :all_files, :changed_files, as: :reversed_boolean)
+        read_tool_option(opts, tool_name, :changed_files, :limit_targets, as: :boolean)
+        read_tool_option(opts, tool_name, :all_files, :limit_targets, as: :reversed_boolean)
         read_tool_option(opts, tool_name, :file_filter, :file_filter, as: :string)
       end
 

lib/quiet_quality/version.rb

@@ -1,3 +1,3 @@
 module QuietQuality
-  VERSION = "1.2.0"
+  VERSION = "1.2.1"
 end

spec/quiet_quality/cli/arg_parser_spec.rb

@@ -114,7 +114,7 @@ def self.expect_usage_error(desc, arguments, matcher)
     end
 
     describe "config file options" do
-      expect_options("(none)", [], global: {config_path: nil, skip_config: nil})
+      expect_options("(none)", [], global: {config_path: nil, no_config: nil})
       expect_options("-Cbar.yml", ["-Cbar.yml"], global: {config_path: "bar.yml"})
       expect_options("--config bar.yml", ["--config", "bar.yml"], global: {config_path: "bar.yml"})
       expect_options("-N", ["-N"], global: {no_config: true})
@@ -156,19 +156,19 @@ def self.expect_usage_error(desc, arguments, matcher)
     end
 
     describe "file targeting options" do
-      def self.expect_all_files(desc, arguments, globally:, **tools)
-        tool_args = tools.each_pair.map { |tool, value| [tool, {all_files: value}] }.to_h
-        expect_options(desc, arguments, global: {all_files: globally}, **tool_args)
+      def self.expect_limit_targets(desc, arguments, globally:, **tools)
+        tool_args = tools.each_pair.map { |tool, value| [tool, {limit_targets: value}] }.to_h
+        expect_options(desc, arguments, global: {limit_targets: globally}, **tool_args)
       end
 
-      expect_all_files("nothing", [], globally: nil, standardrb: nil, rubocop: nil, rspec: nil)
-      expect_all_files("--all-files", ["--all-files"], globally: true)
-      expect_all_files("-a", ["-a"], globally: true)
-      expect_all_files("--changed-files", ["--changed-files"], globally: false)
-      expect_all_files("-c", ["-c"], globally: false)
-      expect_all_files("--all-files standardrb", ["--all-files", "standardrb"], globally: nil, standardrb: true, rubocop: nil, rspec: nil)
-      expect_all_files("-a -crspec", ["-a", "-crspec"], globally: true, rspec: false, standardrb: nil, rubocop: nil)
-      expect_all_files("-arspec -crubocop", ["-arspec", "-crubocop"], globally: nil, rspec: true, rubocop: false, standardrb: nil)
+      expect_limit_targets("nothing", [], globally: nil, standardrb: nil, rubocop: nil, rspec: nil)
+      expect_limit_targets("--all-files", ["--all-files"], globally: false)
+      expect_limit_targets("-a", ["-a"], globally: false)
+      expect_limit_targets("--changed-files", ["--changed-files"], globally: true)
+      expect_limit_targets("-c", ["-c"], globally: true)
+      expect_limit_targets("--all-files standardrb", ["--all-files", "standardrb"], globally: nil, standardrb: false, rubocop: nil, rspec: nil)
+      expect_limit_targets("-a -crspec", ["-a", "-crspec"], globally: false, rspec: true, standardrb: nil, rubocop: nil)
+      expect_limit_targets("-arspec -crubocop", ["-arspec", "-crubocop"], globally: nil, rspec: false, rubocop: true, standardrb: nil)
 
       expect_usage_error("--all-files fooba", ["--all-files", "fooba"], /Unrecognized tool/)
       expect_usage_error("-afooba", ["-afooba"], /Unrecognized tool/)

spec/quiet_quality/config/parsed_options_spec.rb

@@ -1,6 +1,9 @@
 RSpec.describe QuietQuality::Config::ParsedOptions do
   subject(:parsed_options) { described_class.new }
 
+  before { stub_const("QuietQuality::Config::ParsedOptions::GLOBAL_OPTIONS", [:foo, :bar].to_set) }
+  before { stub_const("QuietQuality::Config::ParsedOptions::TOOL_OPTIONS", [:zam, :zim, :foo].to_set) }
+
   it { is_expected.to respond_to(:tools) }
   it { is_expected.to respond_to(:tools=) }
 
@@ -42,18 +45,28 @@
         .from(nil).to(:bar)
     end
 
+    it "cannot be set with an unexpected name" do
+      expect { parsed_options.set_global_option(:baz, "val") }
+        .to raise_error(described_class::InvalidOptionName)
+    end
+
+    it "cannot be fetched with an unexpected name" do
+      expect { parsed_options.global_option(:baz) }
+        .to raise_error(described_class::InvalidOptionName)
+    end
+
     it "can be set multiple times" do
       parsed_options.set_global_option(:foo, :bar)
       parsed_options.set_global_option(:foo, :baz)
       expect(parsed_options.global_option(:foo)).to eq(:baz)
     end
 
     it "can be set and retrieved with strings or symbols" do
-      parsed_options.set_global_option("foo1", "bar1")
-      expect(parsed_options.global_option(:foo1)).to eq("bar1")
+      parsed_options.set_global_option("foo", "bar1")
+      expect(parsed_options.global_option(:foo)).to eq("bar1")
 
-      parsed_options.set_global_option(:foo2, "bar2")
-      expect(parsed_options.global_option("foo2")).to eq("bar2")
+      parsed_options.set_global_option(:bar, "bar2")
+      expect(parsed_options.global_option("bar")).to eq("bar2")
     end
 
     it "can store and retrieve falsey values" do
@@ -64,39 +77,49 @@
 
   describe "a tool option" do
     it "is nil until set" do
-      expect(parsed_options.tool_option(:spade, :foo)).to be_nil
-      expect(parsed_options.tool_option(:hammer, :bar)).to be_nil
+      expect(parsed_options.tool_option(:spade, :zam)).to be_nil
+      expect(parsed_options.tool_option(:hammer, :zim)).to be_nil
     end
 
     it "can be set and then retrieved" do
-      parsed_options.set_tool_option(:spade, :foo, true)
-      expect(parsed_options.tool_option(:spade, :foo)).to eq(true)
+      parsed_options.set_tool_option(:spade, :zam, true)
+      expect(parsed_options.tool_option(:spade, :zam)).to eq(true)
+    end
+
+    it "cannot be set with an unexpected name" do
+      expect { parsed_options.set_tool_option(:spade, :blargh, true) }
+        .to raise_error(described_class::InvalidOptionName)
+    end
+
+    it "cannot be fetched with an unexpected name" do
+      expect { parsed_options.tool_option(:spade, :blargh) }
+        .to raise_error(described_class::InvalidOptionName)
     end
 
     it "can be set multiple times" do
-      parsed_options.set_tool_option(:spade, :foo, :bar)
-      parsed_options.set_tool_option(:spade, :foo, :baz)
-      expect(parsed_options.tool_option(:spade, :foo)).to eq(:baz)
+      parsed_options.set_tool_option(:spade, :zam, :bar)
+      parsed_options.set_tool_option(:spade, :zam, :baz)
+      expect(parsed_options.tool_option(:spade, :zam)).to eq(:baz)
     end
 
     it "can be set with strings and then retrieved with symbols" do
-      parsed_options.set_tool_option("spade", "foo1", "bar1")
-      expect(parsed_options.tool_option(:spade, :foo1)).to eq("bar1")
+      parsed_options.set_tool_option("spade", "zam", "bar1")
+      expect(parsed_options.tool_option(:spade, :zam)).to eq("bar1")
 
-      parsed_options.set_tool_option(:spade, :foo1, "bar2")
-      expect(parsed_options.tool_option("spade", "foo1")).to eq("bar2")
+      parsed_options.set_tool_option(:spade, :zim, "bar2")
+      expect(parsed_options.tool_option("spade", "zim")).to eq("bar2")
     end
 
     it "can store and retrieve falsey values" do
-      parsed_options.set_tool_option(:spade, :foo, false)
-      expect(parsed_options.tool_option(:spade, :foo)).to eq(false)
+      parsed_options.set_tool_option(:spade, :zam, false)
+      expect(parsed_options.tool_option(:spade, :zam)).to eq(false)
     end
 
     it "can be set differently for different tools" do
-      parsed_options.set_tool_option(:spade, :foo, true)
-      parsed_options.set_tool_option(:hammer, :foo, false)
-      expect(parsed_options.tool_option(:spade, :foo)).to eq(true)
-      expect(parsed_options.tool_option(:hammer, :foo)).to eq(false)
+      parsed_options.set_tool_option(:spade, :zam, true)
+      parsed_options.set_tool_option(:hammer, :zam, false)
+      expect(parsed_options.tool_option(:spade, :zam)).to eq(true)
+      expect(parsed_options.tool_option(:hammer, :zam)).to eq(false)
     end
 
     it "is not affected by a matching global option" do

spec/quiet_quality/config/parser_spec.rb

@@ -56,14 +56,14 @@ def self.expect_config(description, config, defaults: nil, globals: nil, tools:
         executor: :concurrent,
         annotator: nil,
         comparison_branch: "master",
-        changed_files: true,
+        limit_targets: true,
         filter_messages: false,
         logging: :light
       )
       expect_tool_options(
-        rspec: {filter_messages: false, changed_files: false, file_filter: "spec/.*_spec.rb"},
-        standardrb: {filter_messages: true, changed_files: nil, file_filter: nil},
-        rubocop: {filter_messages: nil, changed_files: false, file_filter: nil}
+        rspec: {filter_messages: false, limit_targets: false, file_filter: "spec/.*_spec.rb"},
+        standardrb: {filter_messages: true, limit_targets: nil, file_filter: nil},
+        rubocop: {filter_messages: nil, limit_targets: false, file_filter: nil}
       )
     end
 
@@ -75,7 +75,7 @@ def self.expect_config(description, config, defaults: nil, globals: nil, tools:
       context "that is simple but correct" do
         let(:yaml) { "{changed_files: true}" }
         it { is_expected.to be_a(QuietQuality::Config::ParsedOptions) }
-        expect_global_options(changed_files: true, executor: nil, annotator: nil)
+        expect_global_options(limit_targets: true, executor: nil, annotator: nil)
       end
 
       describe "the default_tools parsing" do
@@ -110,13 +110,13 @@ def self.expect_config(description, config, defaults: nil, globals: nil, tools:
       end
 
       describe "changed_files parsing" do
-        expect_config "no settings", %({}), globals: {changed_files: nil}, tools: {rspec: {changed_files: nil}}
-        expect_config "a global changed_files", %({changed_files: true}), globals: {changed_files: true}, tools: {rspec: {changed_files: nil}}
-        expect_config "an rspec changed_files", %({rspec: {changed_files: false}}), globals: {changed_files: nil}, tools: {rspec: {changed_files: false}}
-        expect_config "a global all_files", %({all_files: false}), globals: {changed_files: true}, tools: {rspec: {changed_files: nil}}
-        expect_config "an rspec all_files", %({rspec: {all_files: true}}), globals: {changed_files: nil}, tools: {rspec: {changed_files: false}}
-        expect_config "both changed_files", %({changed_files: true, rspec: {changed_files: false}}), globals: {changed_files: true}, tools: {rspec: {changed_files: false}}
-        expect_config "both all_files", %({all_files: false, rspec: {all_files: true}}), globals: {changed_files: true}, tools: {rspec: {changed_files: false}}
+        expect_config "no settings", %({}), globals: {limit_targets: nil}, tools: {rspec: {limit_targets: nil}}
+        expect_config "a global changed_files", %({changed_files: true}), globals: {limit_targets: true}, tools: {rspec: {limit_targets: nil}}
+        expect_config "an rspec changed_files", %({rspec: {changed_files: false}}), globals: {limit_targets: nil}, tools: {rspec: {limit_targets: false}}
+        expect_config "a global all_files", %({all_files: false}), globals: {limit_targets: true}, tools: {rspec: {limit_targets: nil}}
+        expect_config "an rspec all_files", %({rspec: {all_files: true}}), globals: {limit_targets: nil}, tools: {rspec: {limit_targets: false}}
+        expect_config "both changed_files", %({changed_files: true, rspec: {changed_files: false}}), globals: {limit_targets: true}, tools: {rspec: {limit_targets: false}}
+        expect_config "both all_files", %({all_files: false, rspec: {all_files: true}}), globals: {limit_targets: true}, tools: {rspec: {limit_targets: false}}
         expect_invalid "a non-boolean changed_files", %({changed_files: "yeah"}), /either true or false/
       end