From 3b9c442e0922522a52e890345dc29ec82ec57655 Mon Sep 17 00:00:00 2001 From: Stuart Frost Date: Wed, 26 Jul 2023 14:11:07 +0100 Subject: [PATCH 1/7] Raise error when passed invalid file paths --- spec/ameba/cli/cmd_spec.cr | 20 ++++++++++---------- spec/ameba/glob_utils_spec.cr | 6 ++++++ spec/fixtures/config.yml | 4 ++++ spec/fixtures/passing_ameba.cr | 0 src/ameba/glob_utils.cr | 2 ++ 5 files changed, 22 insertions(+), 10 deletions(-) create mode 100644 spec/fixtures/config.yml create mode 100644 spec/fixtures/passing_ameba.cr diff --git a/spec/ameba/cli/cmd_spec.cr b/spec/ameba/cli/cmd_spec.cr index a412c194..2265a892 100644 --- a/spec/ameba/cli/cmd_spec.cr +++ b/spec/ameba/cli/cmd_spec.cr @@ -5,7 +5,7 @@ module Ameba::Cli describe "Cmd" do describe ".run" do it "runs ameba" do - r = Cli.run %w(-f silent file.cr) + r = Cli.run %w(-f silent -c spec/fixtures/config.yml spec/fixtures/passing_ameba.cr) r.should be_nil end end @@ -43,12 +43,12 @@ module Ameba::Cli end it "defaults rules? flag to false" do - c = Cli.parse_args %w(file.cr) + c = Cli.parse_args %w(spec/fixtures/passing_ameba.cr) c.rules?.should be_false end it "defaults skip_reading_config? flag to false" do - c = Cli.parse_args %w(file.cr) + c = Cli.parse_args %w(spec/fixtures/passing_ameba.cr) c.skip_reading_config?.should be_false end @@ -58,7 +58,7 @@ module Ameba::Cli end it "defaults all? flag to false" do - c = Cli.parse_args %w(file.cr) + c = Cli.parse_args %w(spec/fixtures/passing_ameba.cr) c.all?.should be_false end @@ -95,35 +95,35 @@ module Ameba::Cli describe "-e/--explain" do it "configures file/line/column" do - c = Cli.parse_args %w(--explain src/file.cr:3:5) + c = Cli.parse_args %w(--explain spec/fixtures/passing_ameba.cr:3:5) location_to_explain = c.location_to_explain.should_not be_nil - location_to_explain[:file].should eq "src/file.cr" + location_to_explain[:file].should eq "spec/fixtures/passing_ameba.cr" location_to_explain[:line].should eq 3 location_to_explain[:column].should eq 5 end it "raises an error if location is not valid" do expect_raises(Exception, "location should have PATH:line:column") do - Cli.parse_args %w(--explain src/file.cr:3) + Cli.parse_args %w(--explain spec/fixtures/passing_ameba.cr:3) end end it "raises an error if line number is not valid" do expect_raises(Exception, "location should have PATH:line:column") do - Cli.parse_args %w(--explain src/file.cr:a:3) + Cli.parse_args %w(--explain spec/fixtures/passing_ameba.cr:a:3) end end it "raises an error if column number is not valid" do expect_raises(Exception, "location should have PATH:line:column") do - Cli.parse_args %w(--explain src/file.cr:3:&) + Cli.parse_args %w(--explain spec/fixtures/passing_ameba.cr:3:&) end end it "raises an error if line/column are missing" do expect_raises(Exception, "location should have PATH:line:column") do - Cli.parse_args %w(--explain src/file.cr) + Cli.parse_args %w(--explain spec/fixtures/passing_ameba.cr) end end end diff --git a/spec/ameba/glob_utils_spec.cr b/spec/ameba/glob_utils_spec.cr index 41309195..8481e157 100644 --- a/spec/ameba/glob_utils_spec.cr +++ b/spec/ameba/glob_utils_spec.cr @@ -45,6 +45,12 @@ module Ameba subject.expand(["**/#{current_file_basename}", "**/#{current_file_basename}"]) .should eq [current_file_path] end + + it "raises an ArgumentError when the glob doesn't match any files" do + expect_raises(ArgumentError, "No files found matching foo/*") do + subject.expand(["foo/*"]) + end + end end end end diff --git a/spec/fixtures/config.yml b/spec/fixtures/config.yml new file mode 100644 index 00000000..4cc49722 --- /dev/null +++ b/spec/fixtures/config.yml @@ -0,0 +1,4 @@ +Ameba/PerfRule: + Enabled: false +Ameba/ErrorRule: + Enabled: false diff --git a/spec/fixtures/passing_ameba.cr b/spec/fixtures/passing_ameba.cr new file mode 100644 index 00000000..e69de29b diff --git a/src/ameba/glob_utils.cr b/src/ameba/glob_utils.cr index 71cd0cb5..5a0e5c72 100644 --- a/src/ameba/glob_utils.cr +++ b/src/ameba/glob_utils.cr @@ -21,6 +21,8 @@ module Ameba # ``` def expand(globs) globs.flat_map do |glob| + raise ArgumentError.new("No files found matching #{glob}") if Dir[glob].empty? + glob += "/**/*.cr" if File.directory?(glob) Dir[glob] end.uniq! From e85531df6c7354069438955e3315466c5ce947a3 Mon Sep 17 00:00:00 2001 From: Stuart Frost Date: Thu, 27 Jul 2023 09:24:19 +0100 Subject: [PATCH 2/7] Rename test fixture to source.cr --- spec/ameba/cli/cmd_spec.cr | 20 +++++++++---------- spec/fixtures/{passing_ameba.cr => source.cr} | 0 2 files changed, 10 insertions(+), 10 deletions(-) rename spec/fixtures/{passing_ameba.cr => source.cr} (100%) diff --git a/spec/ameba/cli/cmd_spec.cr b/spec/ameba/cli/cmd_spec.cr index 2265a892..867b6d45 100644 --- a/spec/ameba/cli/cmd_spec.cr +++ b/spec/ameba/cli/cmd_spec.cr @@ -5,7 +5,7 @@ module Ameba::Cli describe "Cmd" do describe ".run" do it "runs ameba" do - r = Cli.run %w(-f silent -c spec/fixtures/config.yml spec/fixtures/passing_ameba.cr) + r = Cli.run %w(-f silent -c spec/fixtures/config.yml spec/fixtures/source.cr) r.should be_nil end end @@ -43,12 +43,12 @@ module Ameba::Cli end it "defaults rules? flag to false" do - c = Cli.parse_args %w(spec/fixtures/passing_ameba.cr) + c = Cli.parse_args %w(spec/fixtures/source.cr) c.rules?.should be_false end it "defaults skip_reading_config? flag to false" do - c = Cli.parse_args %w(spec/fixtures/passing_ameba.cr) + c = Cli.parse_args %w(spec/fixtures/source.cr) c.skip_reading_config?.should be_false end @@ -58,7 +58,7 @@ module Ameba::Cli end it "defaults all? flag to false" do - c = Cli.parse_args %w(spec/fixtures/passing_ameba.cr) + c = Cli.parse_args %w(spec/fixtures/source.cr) c.all?.should be_false end @@ -95,35 +95,35 @@ module Ameba::Cli describe "-e/--explain" do it "configures file/line/column" do - c = Cli.parse_args %w(--explain spec/fixtures/passing_ameba.cr:3:5) + c = Cli.parse_args %w(--explain spec/fixtures/source.cr:3:5) location_to_explain = c.location_to_explain.should_not be_nil - location_to_explain[:file].should eq "spec/fixtures/passing_ameba.cr" + location_to_explain[:file].should eq "spec/fixtures/source.cr" location_to_explain[:line].should eq 3 location_to_explain[:column].should eq 5 end it "raises an error if location is not valid" do expect_raises(Exception, "location should have PATH:line:column") do - Cli.parse_args %w(--explain spec/fixtures/passing_ameba.cr:3) + Cli.parse_args %w(--explain spec/fixtures/source.cr:3) end end it "raises an error if line number is not valid" do expect_raises(Exception, "location should have PATH:line:column") do - Cli.parse_args %w(--explain spec/fixtures/passing_ameba.cr:a:3) + Cli.parse_args %w(--explain spec/fixtures/source.cr:a:3) end end it "raises an error if column number is not valid" do expect_raises(Exception, "location should have PATH:line:column") do - Cli.parse_args %w(--explain spec/fixtures/passing_ameba.cr:3:&) + Cli.parse_args %w(--explain spec/fixtures/source.cr:3:&) end end it "raises an error if line/column are missing" do expect_raises(Exception, "location should have PATH:line:column") do - Cli.parse_args %w(--explain spec/fixtures/passing_ameba.cr) + Cli.parse_args %w(--explain spec/fixtures/source.cr) end end end diff --git a/spec/fixtures/passing_ameba.cr b/spec/fixtures/source.cr similarity index 100% rename from spec/fixtures/passing_ameba.cr rename to spec/fixtures/source.cr From b2069ea4ffd94daeed3b24e4fdd22205660ef460 Mon Sep 17 00:00:00 2001 From: Stuart Frost Date: Thu, 27 Jul 2023 09:29:28 +0100 Subject: [PATCH 3/7] Add extra test cases --- spec/ameba/glob_utils_spec.cr | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/spec/ameba/glob_utils_spec.cr b/spec/ameba/glob_utils_spec.cr index 8481e157..fc47bacd 100644 --- a/spec/ameba/glob_utils_spec.cr +++ b/spec/ameba/glob_utils_spec.cr @@ -51,6 +51,24 @@ module Ameba subject.expand(["foo/*"]) end end + + it "raises an ArgumentError when given a missing file" do + expect_raises(ArgumentError, "No files found matching foo.cr") do + subject.expand(["foo.cr"]) + end + end + + it "raises an ArgumentError when given a missing directory" do + expect_raises(ArgumentError, "No files found matching foo/") do + subject.expand(["foo/"]) + end + end + + it "raises an ArgumentError when given multiple arguments, one of which is missing" do + expect_raises(ArgumentError, "No files found matching foo.cr") do + subject.expand(["**/#{current_file_basename}", "foo.cr"]) + end + end end end end From 7690074cab2ee8d952a907c7ea53de4773013809 Mon Sep 17 00:00:00 2001 From: Stuart Frost Date: Fri, 4 Aug 2023 21:48:35 +0100 Subject: [PATCH 4/7] Conditionally add !lib to default globs --- spec/ameba/config_spec.cr | 20 +++++++++++++++----- src/ameba/config.cr | 9 ++++++++- src/ameba/glob_utils.cr | 3 +-- 3 files changed, 24 insertions(+), 8 deletions(-) diff --git a/spec/ameba/config_spec.cr b/spec/ameba/config_spec.cr index 1c1d10b2..844e88fd 100644 --- a/spec/ameba/config_spec.cr +++ b/spec/ameba/config_spec.cr @@ -9,10 +9,20 @@ module Ameba end describe ".new" do - it "loads default globs when config is empty" do - yml = YAML.parse "{}" - config = Config.new(yml) - config.globs.should eq Config::DEFAULT_GLOBS + context "when config is empty" do + it "loads default globs" do + yml = YAML.parse "{}" + config = Config.new(yml) + config.globs.should eq ["**/*.cr"] + end + + it "only sets !lib as a default glob when there are .cr files in lib" do + File.touch "lib/shard.cr" + yml = YAML.parse "{}" + config = Config.new(yml) + config.globs.should eq Config::DEFAULT_GLOBS + File.delete "lib/shard.cr" + end end it "initializes globs as string" do @@ -102,7 +112,7 @@ module Ameba config = Config.load config_sample it "holds source globs" do - config.globs.should eq Config::DEFAULT_GLOBS + config.globs.should eq ["**/*.cr"] end it "allows to set globs" do diff --git a/src/ameba/config.cr b/src/ameba/config.cr index 821fa065..7d4480c1 100644 --- a/src/ameba/config.cr +++ b/src/ameba/config.cr @@ -95,7 +95,7 @@ class Ameba::Config @rules = Rule.rules.map &.new(config).as(Rule::Base) @rule_groups = @rules.group_by &.group @excluded = load_array_section(config, "Excluded") - @globs = load_array_section(config, "Globs", DEFAULT_GLOBS) + @globs = load_array_section(config, "Globs", default_globs) return unless formatter_name = load_formatter_name(config) self.formatter = formatter_name @@ -239,6 +239,13 @@ class Ameba::Config end end + private def default_globs + DEFAULT_GLOBS.select do |glob| + !Dir[glob].empty? || + (glob.starts_with?("!") && !Dir["#{glob[1..-1]}/**/*.cr"].empty?) + end + end + # :nodoc: module RuleConfig # Define rule properties diff --git a/src/ameba/glob_utils.cr b/src/ameba/glob_utils.cr index 5a0e5c72..df4e80bd 100644 --- a/src/ameba/glob_utils.cr +++ b/src/ameba/glob_utils.cr @@ -21,9 +21,8 @@ module Ameba # ``` def expand(globs) globs.flat_map do |glob| - raise ArgumentError.new("No files found matching #{glob}") if Dir[glob].empty? - glob += "/**/*.cr" if File.directory?(glob) + raise ArgumentError.new("No files found matching #{glob}") if Dir[glob].empty? Dir[glob] end.uniq! end From eb60b25c4e18bba48196ddb926b964ae359e7fef Mon Sep 17 00:00:00 2001 From: Stuart Frost Date: Sat, 5 Aug 2023 16:15:50 +0100 Subject: [PATCH 5/7] Refactor building default globs --- spec/ameba/config_spec.cr | 2 +- src/ameba/config.cr | 10 +++------- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/spec/ameba/config_spec.cr b/spec/ameba/config_spec.cr index 844e88fd..8e5dbb88 100644 --- a/spec/ameba/config_spec.cr +++ b/spec/ameba/config_spec.cr @@ -20,7 +20,7 @@ module Ameba File.touch "lib/shard.cr" yml = YAML.parse "{}" config = Config.new(yml) - config.globs.should eq Config::DEFAULT_GLOBS + config.globs.should eq ["**/*.cr", "!lib"] File.delete "lib/shard.cr" end end diff --git a/src/ameba/config.cr b/src/ameba/config.cr index 7d4480c1..64d81c37 100644 --- a/src/ameba/config.cr +++ b/src/ameba/config.cr @@ -55,10 +55,7 @@ class Ameba::Config Path[XDG_CONFIG_HOME] / "ameba/config.yml", } - DEFAULT_GLOBS = %w( - **/*.cr - !lib - ) + SOURCES_GLOB = "**/*.cr" getter rules : Array(Rule::Base) property severity = Severity::Convention @@ -240,9 +237,8 @@ class Ameba::Config end private def default_globs - DEFAULT_GLOBS.select do |glob| - !Dir[glob].empty? || - (glob.starts_with?("!") && !Dir["#{glob[1..-1]}/**/*.cr"].empty?) + [SOURCES_GLOB].tap do |globs| + globs.push("!lib") if !Dir["lib/**/*.cr"].empty? end end From 1b85ba6f220fca50847751579154e89e5b21e597 Mon Sep 17 00:00:00 2001 From: Stuart Frost Date: Sat, 5 Aug 2023 19:28:47 +0100 Subject: [PATCH 6/7] Refactor: use unless instead of if not --- src/ameba/config.cr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ameba/config.cr b/src/ameba/config.cr index 64d81c37..5030740c 100644 --- a/src/ameba/config.cr +++ b/src/ameba/config.cr @@ -238,7 +238,7 @@ class Ameba::Config private def default_globs [SOURCES_GLOB].tap do |globs| - globs.push("!lib") if !Dir["lib/**/*.cr"].empty? + globs.push("!lib") unless Dir["lib/**/*.cr"].empty? end end From f96cb010150fa9b9dc11b8e5ff2d438f67ca5070 Mon Sep 17 00:00:00 2001 From: Stuart Frost Date: Sat, 5 Aug 2023 19:28:58 +0100 Subject: [PATCH 7/7] Ensure test cleanup occurs --- spec/ameba/config_spec.cr | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/spec/ameba/config_spec.cr b/spec/ameba/config_spec.cr index 8e5dbb88..036c458f 100644 --- a/spec/ameba/config_spec.cr +++ b/spec/ameba/config_spec.cr @@ -16,11 +16,12 @@ module Ameba config.globs.should eq ["**/*.cr"] end - it "only sets !lib as a default glob when there are .cr files in lib" do + it "sets !lib as a default glob when there are .cr files in lib" do File.touch "lib/shard.cr" yml = YAML.parse "{}" config = Config.new(yml) config.globs.should eq ["**/*.cr", "!lib"] + ensure File.delete "lib/shard.cr" end end