From 8b3ca2477444f7d09caf0e4c6de3d8e11d1928a0 Mon Sep 17 00:00:00 2001 From: Eric Mueller Date: Fri, 2 Jun 2023 19:52:59 -0400 Subject: [PATCH 1/6] add a list-validation to limit what can be written to and read from ParsedOptions this breaks a bunch of other tests, but once they're all green the bug will be fixed --- lib/quiet_quality/config/parsed_options.rb | 36 ++++++++++++ .../config/parsed_options_spec.rb | 55 ++++++++++++------- 2 files changed, 70 insertions(+), 21 deletions(-) diff --git a/lib/quiet_quality/config/parsed_options.rb b/lib/quiet_quality/config/parsed_options.rb index 3e24e5b..4dc7e6e 100644 --- a/lib/quiet_quality/config/parsed_options.rb +++ b/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 diff --git a/spec/quiet_quality/config/parsed_options_spec.rb b/spec/quiet_quality/config/parsed_options_spec.rb index b85d4bc..936dfb6 100644 --- a/spec/quiet_quality/config/parsed_options_spec.rb +++ b/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,6 +45,16 @@ .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) @@ -49,11 +62,11 @@ 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,39 @@ 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 "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 From 886115bee60582959893ddfea702661744626f01 Mon Sep 17 00:00:00 2001 From: Eric Mueller Date: Fri, 2 Jun 2023 20:01:16 -0400 Subject: [PATCH 2/6] update ArgParser to write everything to the expected names --- lib/quiet_quality/cli/arg_parser.rb | 14 ++++++------- spec/quiet_quality/cli/arg_parser_spec.rb | 24 +++++++++++------------ 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/lib/quiet_quality/cli/arg_parser.rb b/lib/quiet_quality/cli/arg_parser.rb index afc31eb..0965f6b 100644 --- a/lib/quiet_quality/cli/arg_parser.rb +++ b/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 diff --git a/spec/quiet_quality/cli/arg_parser_spec.rb b/spec/quiet_quality/cli/arg_parser_spec.rb index 0916cdd..f987a1a 100644 --- a/spec/quiet_quality/cli/arg_parser_spec.rb +++ b/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/) From eb70931757a4f04310c776c80ab268205c48340c Mon Sep 17 00:00:00 2001 From: Eric Mueller Date: Fri, 2 Jun 2023 20:05:47 -0400 Subject: [PATCH 3/6] update Config::Parser to write into the correct global/tool parsed options --- lib/quiet_quality/config/parser.rb | 8 ++++---- spec/quiet_quality/config/parser_spec.rb | 24 ++++++++++++------------ 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/lib/quiet_quality/config/parser.rb b/lib/quiet_quality/config/parser.rb index a1a88db..26bc90a 100644 --- a/lib/quiet_quality/config/parser.rb +++ b/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 diff --git a/spec/quiet_quality/config/parser_spec.rb b/spec/quiet_quality/config/parser_spec.rb index 0f5e71f..59e1505 100644 --- a/spec/quiet_quality/config/parser_spec.rb +++ b/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 From b6b8bd27591e6220ced1aada88f1d5017ee06655 Mon Sep 17 00:00:00 2001 From: Eric Mueller Date: Fri, 2 Jun 2023 20:08:51 -0400 Subject: [PATCH 4/6] bump to 1.2.1 --- lib/quiet_quality/version.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/quiet_quality/version.rb b/lib/quiet_quality/version.rb index 24e408b..9091081 100644 --- a/lib/quiet_quality/version.rb +++ b/lib/quiet_quality/version.rb @@ -1,3 +1,3 @@ module QuietQuality - VERSION = "1.2.0" + VERSION = "1.2.1" end From 12a32ddc76fc03a17026e22d13855635ec3807d4 Mon Sep 17 00:00:00 2001 From: Eric Mueller Date: Fri, 2 Jun 2023 20:13:46 -0400 Subject: [PATCH 5/6] add tests covering the unexpected-name case for tool-options thanks simplecov :-) --- spec/quiet_quality/config/parsed_options_spec.rb | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/spec/quiet_quality/config/parsed_options_spec.rb b/spec/quiet_quality/config/parsed_options_spec.rb index 936dfb6..9869a08 100644 --- a/spec/quiet_quality/config/parsed_options_spec.rb +++ b/spec/quiet_quality/config/parsed_options_spec.rb @@ -86,6 +86,16 @@ 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, :zam, :bar) parsed_options.set_tool_option(:spade, :zam, :baz) From 6d0f97256b112451eda660bc06d7b1fc57f16cc0 Mon Sep 17 00:00:00 2001 From: Eric Mueller Date: Sat, 3 Jun 2023 10:54:29 -0400 Subject: [PATCH 6/6] add 1.2.1 to the changelog --- CHANGELOG.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 978c632..3f67987 100644 --- a/CHANGELOG.md +++ b/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