From e0b5c2093abd5fe860b136385bf29b3d76d2c695 Mon Sep 17 00:00:00 2001 From: Rylan Polster Date: Sun, 5 Jul 2020 23:43:09 -0400 Subject: [PATCH] style: refactor OptionDeclarations cop --- Library/Homebrew/dev-cmd/audit.rb | 5 -- Library/Homebrew/rubocops/lines.rb | 52 +++++++++++--------- Library/Homebrew/test/rubocops/lines_spec.rb | 37 ++++++++------ 3 files changed, 50 insertions(+), 44 deletions(-) diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index e25c30e786..48aea66162 100644 --- a/Library/Homebrew/dev-cmd/audit.rb +++ b/Library/Homebrew/dev-cmd/audit.rb @@ -847,11 +847,6 @@ module Homebrew # TODO: check could be in RuboCop problem "`#{Regexp.last_match(1)}` is now unnecessary" if line =~ /(require ["']formula["'])/ - - return unless !@core_tap && line =~ /depends_on .+ if build\.with(out)?\?\(?["']\w+["']\)?/ - - # TODO: check could be in RuboCop - problem "`Use :optional` or `:recommended` instead of `#{Regexp.last_match(0)}`" end def audit_reverse_migration diff --git a/Library/Homebrew/rubocops/lines.rb b/Library/Homebrew/rubocops/lines.rb index 93c9cd3ba9..a2566fb8fb 100644 --- a/Library/Homebrew/rubocops/lines.rb +++ b/Library/Homebrew/rubocops/lines.rb @@ -102,6 +102,33 @@ module RuboCop def audit_formula(_node, _class_node, _parent_class_node, body_node) problem "Use new-style option definitions" if find_method_def(body_node, :options) + if formula_tap == "homebrew-core" + # Use of build.with? implies options, which are forbidden in homebrew/core + find_instance_method_call(body_node, :build, :without?) do + problem "Formulae in homebrew/core should not use `build.without?`." + end + find_instance_method_call(body_node, :build, :with?) do + problem "Formulae in homebrew/core should not use `build.with?`." + end + + 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| + 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]}`" + end + find_instance_method_call(body_node, :build, :without?) do |method| next unless unless_modifier?(method.parent) @@ -143,29 +170,8 @@ module RuboCop problem "Don't duplicate 'with': Use `build.with? \"#{match[1]}\"` to check for \"--with-#{match[1]}\"" end - find_instance_method_call(body_node, :build, :include?) do |method| - arg = parameters(method).first - next unless match = regex_match_group(arg, /^with(out)?-(.*)/) - - problem "Use build.with#{match[1]}? \"#{match[2]}\" instead of " \ - "build.include? 'with#{match[1]}-#{match[2]}'" - end - - find_instance_method_call(body_node, :build, :include?) do |method| - arg = parameters(method).first - next unless match = regex_match_group(arg, /^--(.*)$/) - - problem "Reference '#{match[1]}' without dashes" - end - - return if formula_tap != "homebrew-core" - - # Use of build.with? implies options, which are forbidden in homebrew/core - find_instance_method_call(body_node, :build, :without?) do - problem "Formulae in homebrew/core should not use `build.without?`." - end - find_instance_method_call(body_node, :build, :with?) do - problem "Formulae in homebrew/core should not use `build.with?`." + find_instance_method_call(body_node, :build, :include?) do + problem "`build.include?` is deprecated" end end diff --git a/Library/Homebrew/test/rubocops/lines_spec.rb b/Library/Homebrew/test/rubocops/lines_spec.rb index 95ad56c311..46d24fab94 100644 --- a/Library/Homebrew/test/rubocops/lines_spec.rb +++ b/Library/Homebrew/test/rubocops/lines_spec.rb @@ -201,6 +201,24 @@ describe RuboCop::Cop::FormulaAudit::OptionDeclarations do RUBY end + it "build.without? in dependencies" do + expect_offense(<<~RUBY) + class Foo < Formula + depends_on "bar" if build.without?("baz") + ^^^^^^^^^^^^^^^^^^^^^ Use `:optional` or `:recommended` instead of `if build.without?("baz")` + end + RUBY + end + + it "build.with? in dependencies" do + expect_offense(<<~RUBY) + class Foo < Formula + depends_on "bar" if build.with?("baz") + ^^^^^^^^^^^^^^^^^^ Use `:optional` or `:recommended` instead of `if build.with?("baz")` + end + RUBY + end + it "unless build.without? conditional" do expect_offense(<<~RUBY) class Foo < Formula @@ -279,27 +297,14 @@ describe RuboCop::Cop::FormulaAudit::OptionDeclarations do RUBY end - it "build.include? conditional" do + it "build.include? deprecated" do expect_offense(<<~RUBY) class Foo < Formula desc "foo" url 'https://brew.sh/foo-1.0.tgz' def post_install - return if build.include? "without-bar" - ^^^^^^^^^^^ Use build.without? \"bar\" instead of build.include? 'without-bar' - end - end - RUBY - end - - it "build.include? with dashed args conditional" do - expect_offense(<<~RUBY) - class Foo < Formula - desc "foo" - url 'https://brew.sh/foo-1.0.tgz' - def post_install - return if build.include? "--bar" - ^^^^^ Reference 'bar' without dashes + return if build.include? "foo" + ^^^^^^^^^^^^^^^^^^^^ `build.include?` is deprecated end end RUBY