From 1859162735cee39438253e51ef66601efc9c666a Mon Sep 17 00:00:00 2001 From: Rylan Polster Date: Sun, 5 Jul 2020 19:58:26 -0400 Subject: [PATCH] style: use pkgshare instead of share/foo --- Library/Homebrew/dev-cmd/audit.rb | 16 +--- Library/Homebrew/rubocops/text.rb | 14 +++ Library/Homebrew/test/dev-cmd/audit_spec.rb | 54 ------------ Library/Homebrew/test/rubocops/text_spec.rb | 96 +++++++++++++++++++++ 4 files changed, 113 insertions(+), 67 deletions(-) diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index 32927b085f..e25c30e786 100644 --- a/Library/Homebrew/dev-cmd/audit.rb +++ b/Library/Homebrew/dev-cmd/audit.rb @@ -848,20 +848,10 @@ module Homebrew # TODO: check could be in RuboCop problem "`#{Regexp.last_match(1)}` is now unnecessary" if line =~ /(require ["']formula["'])/ - if line.match?(%r{#\{share\}/#{Regexp.escape(formula.name)}[/'"]}) - # TODO: check could be in RuboCop - problem "Use \#{pkgshare} instead of \#{share}/#{formula.name}" - end + return unless !@core_tap && line =~ /depends_on .+ if build\.with(out)?\?\(?["']\w+["']\)?/ - if !@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 - - if line =~ %r{share(\s*[/+]\s*)(['"])#{Regexp.escape(formula.name)}(?:\2|/)} - # TODO: check could be in RuboCop - problem "Use pkgshare instead of (share#{Regexp.last_match(1)}\"#{formula.name}\")" - end + # 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/text.rb b/Library/Homebrew/rubocops/text.rb index aa9ea3a726..28456e8e93 100644 --- a/Library/Homebrew/rubocops/text.rb +++ b/Library/Homebrew/rubocops/text.rb @@ -121,6 +121,20 @@ 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]}`" + 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 + end + return unless formula_tap == "homebrew-core" find_method_with_args(body_node, :env, :std) do diff --git a/Library/Homebrew/test/dev-cmd/audit_spec.rb b/Library/Homebrew/test/dev-cmd/audit_spec.rb index a7b770592f..75a5fa8af8 100644 --- a/Library/Homebrew/test/dev-cmd/audit_spec.rb +++ b/Library/Homebrew/test/dev-cmd/audit_spec.rb @@ -179,60 +179,6 @@ module Homebrew end end - # Intentionally outputted non-interpolated strings - # rubocop:disable Lint/InterpolationCheck - describe "#line_problems" do - specify "pkgshare" do - fa = formula_auditor "foo", <<~RUBY, strict: true - class Foo < Formula - url "https://brew.sh/foo-1.0.tgz" - end - RUBY - - fa.line_problems 'ohai "#{share}/foo"', 3 - expect(fa.problems.shift).to eq("Use \#{pkgshare} instead of \#{share}/foo") - - fa.line_problems 'ohai "#{share}/foo/bar"', 3 - expect(fa.problems.shift).to eq("Use \#{pkgshare} instead of \#{share}/foo") - - fa.line_problems 'ohai share/"foo"', 3 - expect(fa.problems.shift).to eq('Use pkgshare instead of (share/"foo")') - - fa.line_problems 'ohai share/"foo/bar"', 3 - expect(fa.problems.shift).to eq('Use pkgshare instead of (share/"foo")') - - fa.line_problems 'ohai "#{share}/foo-bar"', 3 - expect(fa.problems).to eq([]) - - fa.line_problems 'ohai share/"foo-bar"', 3 - expect(fa.problems).to eq([]) - - fa.line_problems 'ohai share/"bar"', 3 - expect(fa.problems).to eq([]) - end - - # Regression test for https://github.com/Homebrew/legacy-homebrew/pull/48744 - # Formulae with "++" in their name would break various audit regexps: - # Error: nested *?+ in regexp: /^libxml++3\s/ - specify "++ in name" do - fa = formula_auditor "foolibc++", <<~RUBY, strict: true - class Foolibcxx < Formula - desc "foolibc++ is a test" - url "https://brew.sh/foo-1.0.tgz" - end - RUBY - - fa.line_problems 'ohai "#{share}/foolibc++"', 3 - expect(fa.problems.shift) - .to eq("Use \#{pkgshare} instead of \#{share}/foolibc++") - - fa.line_problems 'ohai share/"foolibc++"', 3 - expect(fa.problems.shift) - .to eq('Use pkgshare instead of (share/"foolibc++")') - end - end - # rubocop:enable Lint/InterpolationCheck - describe "#audit_github_repository" do specify "#audit_github_repository when HOMEBREW_NO_GITHUB_API is set" do ENV["HOMEBREW_NO_GITHUB_API"] = "1" diff --git a/Library/Homebrew/test/rubocops/text_spec.rb b/Library/Homebrew/test/rubocops/text_spec.rb index 82545b3fc1..c357375ae4 100644 --- a/Library/Homebrew/test/rubocops/text_spec.rb +++ b/Library/Homebrew/test/rubocops/text_spec.rb @@ -342,5 +342,101 @@ describe RuboCop::Cop::FormulaAuditStrict::Text do end RUBY end + + it "when `\#{share}/foo` is used instead of `\#{pkgshare}`" do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + def install + ohai "\#{share}/foo" + ^^^^^^^^^^^^^^ Use `\#{pkgshare}` instead of `\#{share}/foo` + end + end + RUBY + end + + it "when `\#{share}/foo/bar` is used instead of `\#{pkgshare}/bar`" do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + def install + ohai "\#{share}/foo/bar" + ^^^^^^^^^^^^^^^^^^ Use `\#{pkgshare}` instead of `\#{share}/foo` + end + end + RUBY + end + + it "when `\#{share}/foolibc++` is used instead of `\#{pkgshare}/foolibc++`" do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foolibc++.rb") + class Foo < Formula + def install + ohai "\#{share}/foolibc++" + ^^^^^^^^^^^^^^^^^^^^ Use `\#{pkgshare}` instead of `\#{share}/foolibc++` + end + end + RUBY + end + + it "when `share/\"foo\"` is used instead of `pkgshare`" do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + def install + ohai share/"foo" + ^^^^^^^^^^^ Use `pkgshare` instead of `share/"foo"` + end + end + RUBY + end + + it "when `share/\"foo/bar\"` is used instead of `pkgshare`" do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + def install + ohai share/"foo/bar" + ^^^^^^^^^^^^^^^ Use `pkgshare` instead of `share/"foo"` + end + end + RUBY + end + + it "when `share/\"foolibc++\"` is used instead of `pkgshare`" do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foolibc++.rb") + class Foo < Formula + def install + ohai share/"foolibc++" + ^^^^^^^^^^^^^^^^^ Use `pkgshare` instead of `share/"foolibc++"` + end + end + RUBY + end + + it "when `\#{share}/foo-bar` doesn't match formula name" do + expect_no_offenses(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + def install + ohai "\#{share}/foo-bar" + end + end + RUBY + end + + it "when `share/foo-bar` doesn't match formula name" do + expect_no_offenses(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + def install + ohai share/"foo-bar" + end + end + RUBY + end + + it "when `share/bar` doesn't match formula name" do + expect_no_offenses(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo < Formula + def install + ohai share/"bar" + end + end + RUBY + end end end