From 074f79840c0ea6e77bda4d581ecaadf6d81dee17 Mon Sep 17 00:00:00 2001 From: Rylan Polster Date: Sat, 11 Jul 2020 17:10:39 -0400 Subject: [PATCH 1/6] style: refactor prefix path check --- Library/Homebrew/rubocops/text.rb | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/Library/Homebrew/rubocops/text.rb b/Library/Homebrew/rubocops/text.rb index 85f0a89ba4..04fbdab3e2 100644 --- a/Library/Homebrew/rubocops/text.rb +++ b/Library/Homebrew/rubocops/text.rb @@ -106,20 +106,18 @@ module RuboCop problem "Use `depends_on :java` to set JAVA_HOME" end - find_strings(body_node).each do |n| - # Skip strings that don't start with one of the keywords - next unless regex_match_group(n, %r{^(bin|include|libexec|lib|sbin|share|Frameworks)/?}) + prefix_path(body_node) do |prefix_node, path| + next unless match = path.match(%r{^(bin|include|libexec|lib|sbin|share|Frameworks)(?:/| |$)}) - parent = n.parent - # Only look at keywords that have `prefix` before them - # TODO: this should be refactored to a direct method match - prefix_keyword_regex = %r{(prefix\s*\+\s*["'](bin|include|libexec|lib|sbin|share|Frameworks))["'/]} - if match = parent.source.match(prefix_keyword_regex) - offending_node(parent) - problem "Use `#{match[2].downcase}` instead of `#{match[1]}\"`" - end + offending_node(prefix_node) + problem "Use `#{match[1].downcase}` instead of `prefix + \"#{match[1]}\"`" end end + + # Find: prefix + "foo" + def_node_search :prefix_path, <<~EOS + $(send (send nil? :prefix) :+ (str $_)) + EOS end end From 8c0c713d6b0b957a050afc3d7837e60e8f0cd4c9 Mon Sep 17 00:00:00 2001 From: Rylan Polster Date: Sat, 11 Jul 2020 17:41:58 -0400 Subject: [PATCH 2/6] style: don't use build.with? for dependencies --- Library/Homebrew/rubocops/lines.rb | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/Library/Homebrew/rubocops/lines.rb b/Library/Homebrew/rubocops/lines.rb index 0310b94348..5fc7258015 100644 --- a/Library/Homebrew/rubocops/lines.rb +++ b/Library/Homebrew/rubocops/lines.rb @@ -114,20 +114,9 @@ module RuboCop return end - # Matches `depends_on "foo" if build.with?("bar")` or depends_on "foo" if build.without?("bar")` - depends_on_build_regex = /depends_on .+ (if build\.with(out)?\?\(["']\w+["']\))/ - - find_instance_method_call(body_node, :build, :with?) do |n| - # TODO: this should be refactored to a direct method match - next unless match = n.parent.source.match(depends_on_build_regex) - - problem "Use `:optional` or `:recommended` instead of `#{match[1]}`" - end - - find_instance_method_call(body_node, :build, :without?) do |n| - next unless match = n.parent.source.match(depends_on_build_regex) - - problem "Use `:optional` or `:recommended` instead of `#{match[1]}`" + depends_on_build_with(body_node) do |build_with_node| + offending_node(build_with_node) + problem "Use `:optional` or `:recommended` instead of `if #{build_with_node.source}`" end find_instance_method_call(body_node, :build, :without?) do |method| @@ -181,6 +170,11 @@ module RuboCop node.modifier_form? && node.unless? end + + # Finds `depends_on "foo" if build.with?("bar")` or `depends_on "foo" if build.without?("bar")` + def_node_search :depends_on_build_with, <<~EOS + (if $(send (send nil? :build) {:with? :without?} str) (send nil? :depends_on str) nil?) + EOS end class MpiCheck < FormulaCop From 6119934efb76b9f88d79ebe7e2d4c729bfd371b5 Mon Sep 17 00:00:00 2001 From: Rylan Polster Date: Sat, 11 Jul 2020 18:07:06 -0400 Subject: [PATCH 3/6] style: refactor pkgshare cop --- Library/Homebrew/rubocops/text.rb | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/Library/Homebrew/rubocops/text.rb b/Library/Homebrew/rubocops/text.rb index 04fbdab3e2..d925106240 100644 --- a/Library/Homebrew/rubocops/text.rb +++ b/Library/Homebrew/rubocops/text.rb @@ -132,18 +132,14 @@ module RuboCop problem "`env :userpaths` in homebrew/core formulae is deprecated" end - body_node.each_descendant(:dstr) do |dstr_node| - next unless match = dstr_node.source.match(%r{(\#{share}/#{Regexp.escape(@formula_name)})[ /"]}) - - offending_node(dstr_node) - problem "Use `\#{pkgshare}` instead of `#{match[1]}`" + share_formula_name(body_node) do |share_node| + offending_node(share_node) + problem "Use `pkgshare` instead of `share/\"#{@formula_name}\"`" end - find_every_method_call_by_name(body_node, :share).each do |share_node| - if match = share_node.parent.source.match(%r{(share\s*[/+]\s*"#{Regexp.escape(@formula_name)})[/"]}) - offending_node(share_node.parent) - problem "Use `pkgshare` instead of `#{match[1]}\"`" - end + share_formula_name_dstr(body_node) do |share_node| + offending_node(share_node) + problem "Use `\#{pkgshare}` instead of `\#{share}/#{@formula_name}`" end return unless formula_tap == "homebrew-core" @@ -152,6 +148,21 @@ module RuboCop problem "`env :std` in homebrew/core formulae is deprecated" end end + + # Check whether value starts with the formula name and then a "/", " " or EOS + def starts_with_formula_name?(value) + value.match?(%r{#{Regexp.escape(@formula_name)}(/| |$)}) + end + + # Find "#{share}/foo" + def_node_search :share_formula_name_dstr, <<~EOS + $(dstr (begin (send nil? :share)) (str #starts_with_formula_name?)) + EOS + + # Find share/"foo" + def_node_search :share_formula_name, <<~EOS + $(send (send nil? :share) :/ (str #starts_with_formula_name?)) + EOS end end end From 55246d720e9e8408c76741bb5c104f3df0aff789 Mon Sep 17 00:00:00 2001 From: Rylan Polster Date: Sun, 12 Jul 2020 14:20:05 -0400 Subject: [PATCH 4/6] improve style in pattern matcher --- Library/Homebrew/rubocops/lines.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Library/Homebrew/rubocops/lines.rb b/Library/Homebrew/rubocops/lines.rb index 5fc7258015..552225fb5b 100644 --- a/Library/Homebrew/rubocops/lines.rb +++ b/Library/Homebrew/rubocops/lines.rb @@ -173,7 +173,8 @@ module RuboCop # Finds `depends_on "foo" if build.with?("bar")` or `depends_on "foo" if build.without?("bar")` def_node_search :depends_on_build_with, <<~EOS - (if $(send (send nil? :build) {:with? :without?} str) (send nil? :depends_on str) nil?) + (if $(send (send nil? :build) {:with? :without?} str) + (send nil? :depends_on str) nil?) EOS end From d8fb850fa9fd535a0e08149b4faed2a1ce73a44a Mon Sep 17 00:00:00 2001 From: Rylan Polster Date: Sun, 12 Jul 2020 14:20:50 -0400 Subject: [PATCH 5/6] fix pkgshare missing slash issue --- Library/Homebrew/rubocops/text.rb | 6 +++--- Library/Homebrew/test/rubocops/text_spec.rb | 20 ++++++++++++++++++++ 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/Library/Homebrew/rubocops/text.rb b/Library/Homebrew/rubocops/text.rb index d925106240..1e0c2ab7d6 100644 --- a/Library/Homebrew/rubocops/text.rb +++ b/Library/Homebrew/rubocops/text.rb @@ -150,13 +150,13 @@ module RuboCop end # Check whether value starts with the formula name and then a "/", " " or EOS - def starts_with_formula_name?(value) - value.match?(%r{#{Regexp.escape(@formula_name)}(/| |$)}) + def starts_with_formula_name?(value, prefix = "") + value.match?(%r{^#{Regexp.escape(prefix + @formula_name)}(/| |$)}) end # Find "#{share}/foo" def_node_search :share_formula_name_dstr, <<~EOS - $(dstr (begin (send nil? :share)) (str #starts_with_formula_name?)) + $(dstr (begin (send nil? :share)) (str #starts_with_formula_name?("/"))) EOS # Find share/"foo" diff --git a/Library/Homebrew/test/rubocops/text_spec.rb b/Library/Homebrew/test/rubocops/text_spec.rb index 4ccb551145..3960272dbc 100644 --- a/Library/Homebrew/test/rubocops/text_spec.rb +++ b/Library/Homebrew/test/rubocops/text_spec.rb @@ -449,5 +449,25 @@ describe RuboCop::Cop::FormulaAuditStrict::Text do end RUBY end + + it "when formula name appears afer `share/\"bar\"`" do + expect_no_offenses(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + def install + ohai share/"bar/foo" + end + end + RUBY + end + + it "when formula name appears afer `\"\#{share}/bar\"`" do + expect_no_offenses(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + def install + ohai "\#{share}/bar/foo" + end + end + RUBY + end end end From 936f4bbff4ac1d619079b40f568103fabf8a180d Mon Sep 17 00:00:00 2001 From: Rylan Polster Date: Sun, 12 Jul 2020 16:06:50 -0400 Subject: [PATCH 6/6] refactor share path node matcher --- Library/Homebrew/rubocops/text.rb | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/Library/Homebrew/rubocops/text.rb b/Library/Homebrew/rubocops/text.rb index 1e0c2ab7d6..49dece4dd4 100644 --- a/Library/Homebrew/rubocops/text.rb +++ b/Library/Homebrew/rubocops/text.rb @@ -132,12 +132,12 @@ module RuboCop problem "`env :userpaths` in homebrew/core formulae is deprecated" end - share_formula_name(body_node) do |share_node| + share_path_starts_with(body_node, @formula_name) do |share_node| offending_node(share_node) problem "Use `pkgshare` instead of `share/\"#{@formula_name}\"`" end - share_formula_name_dstr(body_node) do |share_node| + interpolated_share_path_starts_with(body_node, "/#{@formula_name}") do |share_node| offending_node(share_node) problem "Use `\#{pkgshare}` instead of `\#{share}/#{@formula_name}`" end @@ -150,18 +150,18 @@ module RuboCop end # Check whether value starts with the formula name and then a "/", " " or EOS - def starts_with_formula_name?(value, prefix = "") - value.match?(%r{^#{Regexp.escape(prefix + @formula_name)}(/| |$)}) + def path_starts_with?(path, starts_with) + path.match?(%r{^#{Regexp.escape(starts_with)}(/| |$)}) end # Find "#{share}/foo" - def_node_search :share_formula_name_dstr, <<~EOS - $(dstr (begin (send nil? :share)) (str #starts_with_formula_name?("/"))) + def_node_search :interpolated_share_path_starts_with, <<~EOS + $(dstr (begin (send nil? :share)) (str #path_starts_with?(%1))) EOS # Find share/"foo" - def_node_search :share_formula_name, <<~EOS - $(send (send nil? :share) :/ (str #starts_with_formula_name?)) + def_node_search :share_path_starts_with, <<~EOS + $(send (send nil? :share) :/ (str #path_starts_with?(%1))) EOS end end