diff options
37 files changed, 1159 insertions, 1382 deletions
diff --git a/Library/.rubocop.yml b/Library/.rubocop.yml index d1a3860db..00956076a 100644 --- a/Library/.rubocop.yml +++ b/Library/.rubocop.yml @@ -123,6 +123,15 @@ Style/Documentation: Style/Encoding: Enabled: true +# use spaces for indentation; detect tabs +Layout/Tab: + Enabled: true + +# We have no use for using `warn` because we are +# calling Ruby with warnings disabled ourselves. +Style/StderrPuts: + Enabled: false + # dashes in filenames are typical Naming/FileName: Regex: !ruby/regexp /^[\w\@\-\+\.]+(\.rb)?$/ diff --git a/Library/Homebrew/brew.rb b/Library/Homebrew/brew.rb index 8c5386612..2cc339a83 100644 --- a/Library/Homebrew/brew.rb +++ b/Library/Homebrew/brew.rb @@ -78,7 +78,8 @@ begin # - a help flag is passed AND a command is matched # - a help flag is passed AND there is no command specified # - no arguments are passed - if empty_argv || help_flag + # - if cmd is Cask, let Cask handle the help command instead + if (empty_argv || help_flag) && cmd != "cask" require "cmd/help" Homebrew.help cmd, empty_argv: empty_argv # `Homebrew.help` never returns, except for external/unknown commands. @@ -116,8 +117,11 @@ begin if Process.uid.zero? && !brew_uid.zero? tap_commands += %W[/usr/bin/sudo -u ##{brew_uid}] end + # Unset HOMEBREW_HELP to avoid confusing the tap + ENV.delete("HOMEBREW_HELP") if help_flag tap_commands += %W[#{HOMEBREW_BREW_FILE} tap #{possible_tap}] safe_system(*tap_commands) + ENV["HOMEBREW_HELP"] = "1" if help_flag exec HOMEBREW_BREW_FILE, cmd, *ARGV end rescue UsageError => e diff --git a/Library/Homebrew/build_environment.rb b/Library/Homebrew/build_environment.rb index dc28b2293..3c494428f 100644 --- a/Library/Homebrew/build_environment.rb +++ b/Library/Homebrew/build_environment.rb @@ -46,6 +46,7 @@ module Homebrew HOMEBREW_SDKROOT HOMEBREW_BUILD_FROM_SOURCE MAKE GIT CPP ACLOCAL_PATH PATH CPATH + LD_LIBRARY_PATH LD_RUN_PATH LD_PRELOAD LIBRARY_PATH ].select { |key| env.key?(key) } end diff --git a/Library/Homebrew/cask/lib/hbc/cli.rb b/Library/Homebrew/cask/lib/hbc/cli.rb index 9283802d5..e147c8280 100644 --- a/Library/Homebrew/cask/lib/hbc/cli.rb +++ b/Library/Homebrew/cask/lib/hbc/cli.rb @@ -154,7 +154,7 @@ module Hbc def run command_name, *args = detect_command_and_arguments(*@args) command = if help? - args.unshift(command_name) + args.unshift(command_name) unless command_name.nil? "help" else self.class.lookup_command(command_name) @@ -230,8 +230,7 @@ module Hbc return if @command == "help" && @args.empty? - unknown_command = @args.empty? ? @command : @args.first - raise ArgumentError, "Unknown command: #{unknown_command}" + raise ArgumentError, "help does not take arguments." end def purpose diff --git a/Library/Homebrew/caveats.rb b/Library/Homebrew/caveats.rb index 485116cff..49a517bd4 100644 --- a/Library/Homebrew/caveats.rb +++ b/Library/Homebrew/caveats.rb @@ -1,3 +1,5 @@ +require "forwardable" + class Caveats extend Forwardable diff --git a/Library/Homebrew/cleanup.rb b/Library/Homebrew/cleanup.rb index 340161204..960bcb8b7 100644 --- a/Library/Homebrew/cleanup.rb +++ b/Library/Homebrew/cleanup.rb @@ -1,6 +1,5 @@ require "utils/bottles" require "formula" -require "thread" module Homebrew module Cleanup diff --git a/Library/Homebrew/cmd/leaves.rb b/Library/Homebrew/cmd/leaves.rb index 574ceb64e..ecebedbb3 100644 --- a/Library/Homebrew/cmd/leaves.rb +++ b/Library/Homebrew/cmd/leaves.rb @@ -13,16 +13,7 @@ module Homebrew deps_of_installed = Set.new installed.each do |f| - deps = [] - - f.deps.each do |dep| - if dep.optional? || dep.recommended? - deps << dep.to_formula.full_name if f.build.with?(dep) - else - deps << dep.to_formula.full_name - end - end - + deps = f.runtime_dependencies.map { |d| d.to_formula.full_name } deps_of_installed.merge(deps) end diff --git a/Library/Homebrew/constants.rb b/Library/Homebrew/constants.rb index 956548640..529c49feb 100644 --- a/Library/Homebrew/constants.rb +++ b/Library/Homebrew/constants.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true # RuboCop version used for `brew style` and `brew cask style` -HOMEBREW_RUBOCOP_VERSION = "0.50.0" -HOMEBREW_RUBOCOP_CASK_VERSION = "~> 0.14.4" # has to be updated when RuboCop version changes +HOMEBREW_RUBOCOP_VERSION = "0.51.0" +HOMEBREW_RUBOCOP_CASK_VERSION = "~> 0.15.1" # has to be updated when RuboCop version changes diff --git a/Library/Homebrew/dev-cmd/audit.rb b/Library/Homebrew/dev-cmd/audit.rb index ec26eb9cd..bfe4dbc00 100644 --- a/Library/Homebrew/dev-cmd/audit.rb +++ b/Library/Homebrew/dev-cmd/audit.rb @@ -820,168 +820,12 @@ class FormulaAuditor problem "\"(#{Regexp.last_match(1)}...#{Regexp.last_match(2)})\" should be \"(#{Regexp.last_match(3).downcase}+...)\"" end - if line =~ /((man)\s*\+\s*(['"])(man[1-8])(['"]))/ - problem "\"#{Regexp.last_match(1)}\" should be \"#{Regexp.last_match(4)}\"" - end - - # Prefer formula path shortcuts in strings - if line =~ %r[(\#\{prefix\}/(bin|include|libexec|lib|sbin|share|Frameworks))] - problem "\"#{Regexp.last_match(1)}\" should be \"\#{#{Regexp.last_match(2).downcase}}\"" - end - - if line =~ %r[((\#\{prefix\}/share/man/|\#\{man\}/)(man[1-8]))] - problem "\"#{Regexp.last_match(1)}\" should be \"\#{#{Regexp.last_match(3)}}\"" - end - - if line =~ %r[((\#\{share\}/(man)))[/'"]] - problem "\"#{Regexp.last_match(1)}\" should be \"\#{#{Regexp.last_match(3)}}\"" - end - - if line =~ %r[(\#\{prefix\}/share/(info|man))] - problem "\"#{Regexp.last_match(1)}\" should be \"\#{#{Regexp.last_match(2)}}\"" - end - - if line =~ /depends_on\s+['"](.+)['"]\s+=>\s+:(lua|perl|python|ruby)(\d*)/ - problem "#{Regexp.last_match(2)} modules should be vendored rather than use deprecated `depends_on \"#{Regexp.last_match(1)}\" => :#{Regexp.last_match(2)}#{Regexp.last_match(3)}`" - end - - if line =~ /depends_on\s+['"](.+)['"]\s+=>\s+(.*)/ - dep = Regexp.last_match(1) - Regexp.last_match(2).split(" ").map do |o| - break if ["if", "unless"].include?(o) - next unless o =~ /^\[?['"](.*)['"]/ - problem "Dependency #{dep} should not use option #{Regexp.last_match(1)}" - end - end - - if line =~ /if\s+ARGV\.include\?\s+'--(HEAD|devel)'/ - problem "Use \"if build.#{Regexp.last_match(1).downcase}?\" instead" - end - problem "Use separate make calls" if line.include?("make && make") - problem "Use spaces instead of tabs for indentation" if line =~ /^[ ]*\t/ - - if line.include?("ENV.java_cache") - problem "In-formula ENV.java_cache usage has been deprecated & should be removed." - end - - # Avoid hard-coding compilers - if line =~ %r{(system|ENV\[.+\]\s?=)\s?['"](/usr/bin/)?(gcc|llvm-gcc|clang)['" ]} - problem "Use \"\#{ENV.cc}\" instead of hard-coding \"#{Regexp.last_match(3)}\"" - end - - if line =~ %r{(system|ENV\[.+\]\s?=)\s?['"](/usr/bin/)?((g|llvm-g|clang)\+\+)['" ]} - problem "Use \"\#{ENV.cxx}\" instead of hard-coding \"#{Regexp.last_match(3)}\"" - end - - if line =~ /system\s+['"](env|export)(\s+|['"])/ - problem "Use ENV instead of invoking '#{Regexp.last_match(1)}' to modify the environment" - end - - if line =~ /version == ['"]HEAD['"]/ - problem "Use 'build.head?' instead of inspecting 'version'" - end - - if line =~ /build\.include\?[\s\(]+['"]\-\-(.*)['"]/ - problem "Reference '#{Regexp.last_match(1)}' without dashes" - end - - if line =~ /build\.include\?[\s\(]+['"]with(out)?-(.*)['"]/ - problem "Use build.with#{Regexp.last_match(1)}? \"#{Regexp.last_match(2)}\" instead of build.include? 'with#{Regexp.last_match(1)}-#{Regexp.last_match(2)}'" - end - - if line =~ /build\.with\?[\s\(]+['"]-?-?with-(.*)['"]/ - problem "Don't duplicate 'with': Use `build.with? \"#{Regexp.last_match(1)}\"` to check for \"--with-#{Regexp.last_match(1)}\"" - end - - if line =~ /build\.without\?[\s\(]+['"]-?-?without-(.*)['"]/ - problem "Don't duplicate 'without': Use `build.without? \"#{Regexp.last_match(1)}\"` to check for \"--without-#{Regexp.last_match(1)}\"" - end - - if line =~ /unless build\.with\?(.*)/ - problem "Use if build.without?#{Regexp.last_match(1)} instead of unless build.with?#{Regexp.last_match(1)}" - end - - if line =~ /unless build\.without\?(.*)/ - problem "Use if build.with?#{Regexp.last_match(1)} instead of unless build.without?#{Regexp.last_match(1)}" - end - - if line =~ /(not\s|!)\s*build\.with?\?/ - problem "Don't negate 'build.with?': use 'build.without?'" - end - - if line =~ /(not\s|!)\s*build\.without?\?/ - problem "Don't negate 'build.without?': use 'build.with?'" - end - - if line =~ /ARGV\.(?!(debug\?|verbose\?|value[\(\s]))/ - problem "Use build instead of ARGV to check options" - end - - if line.include?("MACOS_VERSION") - problem "Use MacOS.version instead of MACOS_VERSION" - end - - if line.include?("MACOS_FULL_VERSION") - problem "Use MacOS.full_version instead of MACOS_FULL_VERSION" - end - - cats = %w[leopard snow_leopard lion mountain_lion].join("|") - if line =~ /MacOS\.(?:#{cats})\?/ - problem "\"#{$&}\" is deprecated, use a comparison to MacOS.version instead" - end - - if line =~ /depends_on [A-Z][\w:]+\.new$/ - problem "`depends_on` can take requirement classes instead of instances" - end - - if line =~ /^def (\w+).*$/ - problem "Define method #{Regexp.last_match(1).inspect} in the class body, not at the top-level" - end - - if line.include?("ENV.fortran") && !formula.requirements.map(&:class).include?(FortranRequirement) - problem "Use `depends_on :fortran` instead of `ENV.fortran`" - end - if line =~ /JAVA_HOME/i && !formula.requirements.map(&:class).include?(JavaRequirement) problem "Use `depends_on :java` to set JAVA_HOME" end - if line =~ /depends_on :(.+) (if.+|unless.+)$/ - conditional_dep_problems(Regexp.last_match(1).to_sym, Regexp.last_match(2), $&) - end - - if line =~ /depends_on ['"](.+)['"] (if.+|unless.+)$/ - conditional_dep_problems(Regexp.last_match(1), Regexp.last_match(2), $&) - end - - if line =~ /(Dir\[("[^\*{},]+")\])/ - problem "#{Regexp.last_match(1)} is unnecessary; just use #{Regexp.last_match(2)}" - end - - if line =~ /system (["'](#{FILEUTILS_METHODS})["' ])/o - system = Regexp.last_match(1) - method = Regexp.last_match(2) - problem "Use the `#{method}` Ruby method instead of `system #{system}`" - end - - if line =~ /assert [^!]+\.include?/ - problem "Use `assert_match` instead of `assert ...include?`" - end - - if line =~ /(assert File\.exist\?|assert \(.*\)\.exist\?)/ - problem "Use `assert_predicate <path_to_file>, :exist?` instead of `#{Regexp.last_match(1)}`" - end - - if line =~ /(assert !File\.exist\?|assert !\(.*\)\.exist\?)/ - problem "Use `refute_predicate <path_to_file>, :exist?` instead of `#{Regexp.last_match(1)}`" - end - - if line =~ /(assert File\.executable\?|assert \(.*\)\.executable\?)/ - problem "Use `assert_predicate <path_to_file>, :executable?` instead of `#{Regexp.last_match(1)}`" - end - return unless @strict problem "`#{Regexp.last_match(1)}` in formulae is deprecated" if line =~ /(env :(std|userpaths))/ @@ -1028,18 +872,6 @@ class FormulaAuditor EOS end - def conditional_dep_problems(dep, condition, line) - quoted_dep = quote_dep(dep) - dep = Regexp.escape(dep.to_s) - - case condition - when /if build\.include\? ['"]with-#{dep}['"]$/, /if build\.with\? ['"]#{dep}['"]$/ - problem %Q(Replace #{line.inspect} with "depends_on #{quoted_dep} => :optional") - when /unless build\.include\? ['"]without-#{dep}['"]$/, /unless build\.without\? ['"]#{dep}['"]$/ - problem %Q(Replace #{line.inspect} with "depends_on #{quoted_dep} => :recommended") - end - end - def quote_dep(dep) dep.is_a?(Symbol) ? dep.inspect : "'#{dep}'" end diff --git a/Library/Homebrew/diagnostic.rb b/Library/Homebrew/diagnostic.rb index 49402bc07..51a892e28 100644 --- a/Library/Homebrew/diagnostic.rb +++ b/Library/Homebrew/diagnostic.rb @@ -1097,7 +1097,7 @@ module Homebrew def all methods.map(&:to_s).grep(/^check_/) end - end # end class Checks + end end end diff --git a/Library/Homebrew/formulary.rb b/Library/Homebrew/formulary.rb index 7eb85ed97..1b1987717 100644 --- a/Library/Homebrew/formulary.rb +++ b/Library/Homebrew/formulary.rb @@ -422,7 +422,7 @@ module Formulary CoreTap.instance.formula_dir/"#{name.to_s.downcase}.rb" end - def self.tap_paths(name, taps = Dir["#{HOMEBREW_LIBRARY}/Taps/*/*/"]) + def self.tap_paths(name, taps = Dir[HOMEBREW_LIBRARY/"Taps/*/*/"]) name = name.to_s.downcase taps.map do |tap| Pathname.glob([ diff --git a/Library/Homebrew/language/python.rb b/Library/Homebrew/language/python.rb index 931cc59fc..49e3d1a46 100644 --- a/Library/Homebrew/language/python.rb +++ b/Library/Homebrew/language/python.rb @@ -245,7 +245,7 @@ module Language "-v", "--no-deps", "--no-binary", ":all:", "--ignore-installed", *targets end - end # class Virtualenv - end # module Virtualenv - end # module Python -end # module Language + end + end + end +end diff --git a/Library/Homebrew/readall.rb b/Library/Homebrew/readall.rb index 5f4fd947c..1b6ae016e 100644 --- a/Library/Homebrew/readall.rb +++ b/Library/Homebrew/readall.rb @@ -1,6 +1,5 @@ require "formula" require "tap" -require "thread" module Readall class << self diff --git a/Library/Homebrew/rubocops/checksum_cop.rb b/Library/Homebrew/rubocops/checksum_cop.rb index dcaf60e7d..23a787809 100644 --- a/Library/Homebrew/rubocops/checksum_cop.rb +++ b/Library/Homebrew/rubocops/checksum_cop.rb @@ -24,6 +24,7 @@ module RuboCop def audit_sha256(checksum) return if checksum.nil? if regex_match_group(checksum, /^$/) + @offense_source_range = @offensive_node.source_range problem "sha256 is empty" return end diff --git a/Library/Homebrew/rubocops/conflicts_cop.rb b/Library/Homebrew/rubocops/conflicts_cop.rb index 1cca3f8ae..96edf480c 100644 --- a/Library/Homebrew/rubocops/conflicts_cop.rb +++ b/Library/Homebrew/rubocops/conflicts_cop.rb @@ -6,10 +6,8 @@ module RuboCop module FormulaAudit # This cop audits versioned Formulae for `conflicts_with` class Conflicts < FormulaCop - MSG = <<~EOS.freeze - Versioned formulae should not use `conflicts_with`. - Use `keg_only :versioned_formula` instead. - EOS + MSG = "Versioned formulae should not use `conflicts_with`. " \ + "Use `keg_only :versioned_formula` instead.".freeze WHITELIST = %w[ node@ diff --git a/Library/Homebrew/rubocops/extend/formula_cop.rb b/Library/Homebrew/rubocops/extend/formula_cop.rb index 7531e62b7..2e9a7657e 100644 --- a/Library/Homebrew/rubocops/extend/formula_cop.rb +++ b/Library/Homebrew/rubocops/extend/formula_cop.rb @@ -56,6 +56,7 @@ module RuboCop # Returns all string nodes among the descendants of given node def find_strings(node) return [] if node.nil? + return node if node.str_type? node.each_descendant(:str) end @@ -101,7 +102,9 @@ module RuboCop def find_method_with_args(node, method_name, *args) methods = find_every_method_call_by_name(node, method_name) methods.each do |method| - yield method if parameters_passed?(method, *args) + next unless parameters_passed?(method, *args) + return true unless block_given? + yield method end end @@ -119,6 +122,7 @@ module RuboCop method.receiver.method_name != instance @offense_source_range = method.source_range @offensive_node = method + return true unless block_given? yield method end end @@ -167,12 +171,26 @@ module RuboCop type_match && name_match end + # Find CONSTANTs in the source + # if block given, yield matching nodes + def find_const(node, const_name) + return if node.nil? + node.each_descendant(:const) do |const_node| + next unless const_node.const_name == const_name + @offensive_node = const_node + @offense_source_range = const_node.source_range + yield const_node if block_given? + return true + end + nil + end + def_node_search :required_dependency?, <<~EOS - (send nil :depends_on ({str sym} _)) + (send nil? :depends_on ({str sym} _)) EOS def_node_search :required_dependency_name?, <<~EOS - (send nil :depends_on ({str sym} %1)) + (send nil? :depends_on ({str sym} %1)) EOS def_node_search :dependency_type_hash_match?, <<~EOS @@ -222,15 +240,17 @@ module RuboCop end # Returns a method definition node with method_name - def find_method_def(node, method_name) + # Returns first method def if method_name is nil + def find_method_def(node, method_name = nil) return if node.nil? node.each_child_node(:def) do |def_node| def_method_name = method_name(def_node) - next unless method_name == def_method_name + next unless method_name == def_method_name || method_name.nil? @offensive_node = def_node @offense_source_range = def_node.source_range return def_node end + return if node.parent.nil? # If not found then, parent node becomes the offensive node @offensive_node = node.parent @offense_source_range = node.parent.source_range @@ -250,11 +270,15 @@ module RuboCop end # Check if method_name is called among the direct children nodes in the given node + # Check if the node itself is the method def method_called?(node, method_name) + if node.send_type? && node.method_name == method_name + offending_node(node) + return true + end node.each_child_node(:send) do |call_node| next unless call_node.method_name == method_name - @offensive_node = call_node - @offense_source_range = call_node.source_range + offending_node(call_node) return true end false @@ -291,6 +315,11 @@ module RuboCop true end + # Check if negation is present in the given node + def negated?(node) + method_called?(node, :!) + end + # Return all the caveats' string nodes in an array def caveats_strings find_strings(find_method_def(@body, :caveats)) diff --git a/Library/Homebrew/rubocops/formula_desc_cop.rb b/Library/Homebrew/rubocops/formula_desc_cop.rb index 05f60c9d5..e56a4cc56 100644 --- a/Library/Homebrew/rubocops/formula_desc_cop.rb +++ b/Library/Homebrew/rubocops/formula_desc_cop.rb @@ -29,10 +29,8 @@ module RuboCop desc_length = "#{@formula_name}: #{string_content(desc)}".length max_desc_length = 80 return if desc_length <= max_desc_length - problem <<~EOS - Description is too long. "name: desc" should be less than #{max_desc_length} characters. - Length is calculated as #{@formula_name} + desc. (currently #{desc_length}) - EOS + problem "Description is too long. \"name: desc\" should be less than #{max_desc_length} characters. " \ + "Length is calculated as #{@formula_name} + desc. (currently #{desc_length})" end end diff --git a/Library/Homebrew/rubocops/lines_cop.rb b/Library/Homebrew/rubocops/lines_cop.rb index 4172aad96..c5f2e7585 100644 --- a/Library/Homebrew/rubocops/lines_cop.rb +++ b/Library/Homebrew/rubocops/lines_cop.rb @@ -8,11 +8,11 @@ module RuboCop def audit_formula(_node, _class_node, _parent_class_node, _body_node) [:automake, :autoconf, :libtool].each do |dependency| next unless depends_on?(dependency) - problem ":#{dependency} is deprecated. Usage should be \"#{dependency}\"" + problem ":#{dependency} is deprecated. Usage should be \"#{dependency}\"." end - problem ':apr is deprecated. Usage should be "apr-util"' if depends_on?(:apr) - problem ":tex is deprecated" if depends_on?(:tex) + problem ':apr is deprecated. Usage should be "apr-util".' if depends_on?(:apr) + problem ":tex is deprecated." if depends_on?(:tex) end end @@ -21,7 +21,7 @@ module RuboCop begin_pos = start_column(parent_class_node) end_pos = end_column(class_node) return unless begin_pos-end_pos != 3 - problem "Use a space in class inheritance: class #{class_name(class_node)} < #{class_name(parent_class_node)}" + problem "Use a space in class inheritance: class #{@formula_name.capitalize} < #{class_name(parent_class_node)}" end end @@ -53,6 +53,87 @@ module RuboCop end end + class AssertStatements < FormulaCop + def audit_formula(_node, _class_node, _parent_class_node, body_node) + find_every_method_call_by_name(body_node, :assert).each do |method| + if method_called_ever?(method, :include?) && !method_called_ever?(method, :!) + problem "Use `assert_match` instead of `assert ...include?`" + end + + if method_called_ever?(method, :exist?) && !method_called_ever?(method, :!) + problem "Use `assert_predicate <path_to_file>, :exist?` instead of `#{method.source}`" + end + + if method_called_ever?(method, :exist?) && method_called_ever?(method, :!) + problem "Use `refute_predicate <path_to_file>, :exist?` instead of `#{method.source}`" + end + + if method_called_ever?(method, :executable?) && !method_called_ever?(method, :!) + problem "Use `assert_predicate <path_to_file>, :executable?` instead of `#{method.source}`" + end + end + end + end + + class OptionDeclarations < FormulaCop + def audit_formula(_node, _class_node, _parent_class_node, body_node) + if find_method_def(body_node, :options) + problem "Use new-style option definitions" + end + + find_instance_method_call(body_node, :build, :without?) do |method| + next unless unless_modifier?(method.parent) + correct = method.source.gsub("out?", "?") + problem "Use if #{correct} instead of unless #{method.source}" + end + + find_instance_method_call(body_node, :build, :with?) do |method| + next unless unless_modifier?(method.parent) + correct = method.source.gsub("?", "out?") + problem "Use if #{correct} instead of unless #{method.source}" + end + + find_instance_method_call(body_node, :build, :with?) do |method| + next unless negated?(method.parent) + problem "Don't negate 'build.with?': use 'build.without?'" + end + + find_instance_method_call(body_node, :build, :without?) do |method| + next unless negated?(method.parent) + problem "Don't negate 'build.without?': use 'build.with?'" + end + + find_instance_method_call(body_node, :build, :without?) do |method| + arg = parameters(method).first + next unless match = regex_match_group(arg, /-?-?without-(.*)/) + problem "Don't duplicate 'without': Use `build.without? \"#{match[1]}\"` to check for \"--without-#{match[1]}\"" + end + + find_instance_method_call(body_node, :build, :with?) do |method| + arg = parameters(method).first + next unless match = regex_match_group(arg, /-?-?with-(.*)/) + 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 + end + + def unless_modifier?(node) + return false unless node.if_type? + node.modifier_form? && node.unless? + end + end + class Miscellaneous < FormulaCop def audit_formula(_node, _class_node, _parent_class_node, body_node) # FileUtils is included in Formula @@ -81,18 +162,128 @@ module RuboCop end end + [:debug?, :verbose?, :value].each do |method_name| + find_instance_method_call(body_node, "ARGV", method_name) do + problem "Use build instead of ARGV to check options" + end + end + + find_instance_method_call(body_node, :man, :+) do |method| + next unless match = regex_match_group(parameters(method).first, /man[1-8]/) + problem "\"#{method.source}\" should be \"#{match[0]}\"" + end + + # Avoid hard-coding compilers + find_every_method_call_by_name(body_node, :system).each do |method| + param = parameters(method).first + if match = regex_match_group(param, %r{^(/usr/bin/)?(gcc|llvm-gcc|clang)\s?}) + problem "Use \"\#{ENV.cc}\" instead of hard-coding \"#{match[2]}\"" + elsif match = regex_match_group(param, %r{(/usr/bin/)?((g|llvm-g|clang)\+\+)\s?}) + problem "Use \"\#{ENV.cxx}\" instead of hard-coding \"#{match[2]}\"" + end + end + + find_instance_method_call(body_node, "ENV", :[]=) do |method| + param = parameters(method)[1] + if match = regex_match_group(param, %r{^(/usr/bin/)?(gcc|llvm-gcc|clang)\s?}) + problem "Use \"\#{ENV.cc}\" instead of hard-coding \"#{match[2]}\"" + elsif match = regex_match_group(param, %r{^(/usr/bin/)?((g|llvm-g|clang)\+\+)\s?}) + problem "Use \"\#{ENV.cxx}\" instead of hard-coding \"#{match[2]}\"" + end + end + + # Prefer formula path shortcuts in strings + formula_path_strings(body_node, :share) do |p| + next unless match = regex_match_group(p, %r{(/(man))/?}) + problem "\"\#{share}#{match[1]}\" should be \"\#{#{match[2]}}\"" + end + + formula_path_strings(body_node, :prefix) do |p| + if match = regex_match_group(p, %r{(/share/(info|man))$}) + problem "\"\#\{prefix}#{match[1]}\" should be \"\#{#{match[2]}}\"" + end + if match = regex_match_group(p, %r{((/share/man/)(man[1-8]))}) + problem "\"\#\{prefix}#{match[1]}\" should be \"\#{#{match[3]}}\"" + end + if match = regex_match_group(p, %r{(/(bin|include|libexec|lib|sbin|share|Frameworks))}i) + problem "\"\#\{prefix}#{match[1]}\" should be \"\#{#{match[2].downcase}}\"" + end + end + + find_every_method_call_by_name(body_node, :depends_on).each do |method| + key, value = destructure_hash(parameters(method).first) + next if key.nil? || value.nil? + next unless match = regex_match_group(value, /(lua|perl|python|ruby)(\d*)/) + problem "#{match[1]} modules should be vendored rather than use deprecated #{method.source}`" + end + + find_every_method_call_by_name(body_node, :system).each do |method| + next unless match = regex_match_group(parameters(method).first, /(env|export)(\s+)?/) + problem "Use ENV instead of invoking '#{match[1]}' to modify the environment" + end + + find_every_method_call_by_name(body_node, :depends_on).each do |method| + next if modifier?(method.parent) + param = parameters(method).first + dep, option = hash_dep(param) + next if dep.nil? || option.nil? + offending_node(param) + problem "Dependency #{string_content(dep)} should not use option #{string_content(option)}" + end + + find_instance_method_call(body_node, :version, :==) do |method| + next unless parameters_passed?(method, "HEAD") + problem "Use 'build.head?' instead of inspecting 'version'" + end + + find_instance_method_call(body_node, "ENV", :fortran) do + next if depends_on?(:fortran) + problem "Use `depends_on :fortran` instead of `ENV.fortran`" + end + + find_instance_method_call(body_node, "ARGV", :include?) do |method| + param = parameters(method).first + next unless match = regex_match_group(param, /--(HEAD|devel)/) + problem "Use \"if build.#{match[1].downcase}?\" instead" + end + + find_const(body_node, "MACOS_VERSION") do + problem "Use MacOS.version instead of MACOS_VERSION" + end + + find_const(body_node, "MACOS_FULL_VERSION") do + problem "Use MacOS.full_version instead of MACOS_FULL_VERSION" + end + + conditional_dependencies(body_node) do |node, method, param, dep_node| + dep = string_content(dep_node) + if node.if? + if (method == :include? && regex_match_group(param, /with-#{dep}$/)) || + (method == :with? && regex_match_group(param, /#{dep}$/)) + offending_node(dep_node.parent) + problem "Replace #{node.source} with #{dep_node.parent.source} => :optional" + end + elsif node.unless? + if (method == :include? && regex_match_group(param, /without-#{dep}$/)) || + (method == :without? && regex_match_group(param, /#{dep}$/)) + offending_node(dep_node.parent) + problem "Replace #{node.source} with #{dep_node.parent.source} => :recommended" + end + end + end + find_method_with_args(body_node, :fails_with, :llvm) do problem "'fails_with :llvm' is now a no-op so should be removed" end - find_method_with_args(body_node, :system, /^(otool|install_name_tool|lipo)$/) do + find_method_with_args(body_node, :system, /(otool|install_name_tool|lipo)/) do next if @formula_name == "cctools" problem "Use ruby-macho instead of calling #{@offensive_node.source}" end find_every_method_call_by_name(body_node, :system).each do |method_node| # Skip Kibana: npm cache edge (see formula for more details) - next if @formula_name =~ /^kibana(\@\d+(\.\d+)?)?$/ + next if @formula_name =~ /^kibana(\@\d+(\.\d+)?)?$/i first_param, second_param = parameters(method_node) next if !node_equals?(first_param, "npm") || !node_equals?(second_param, "install") @@ -104,15 +295,13 @@ module RuboCop problem "Use new-style test definitions (test do)" end - if find_method_def(body_node, :options) - problem "Use new-style option definitions" + find_method_with_args(body_node, :skip_clean, :all) do + problem "`skip_clean :all` is deprecated; brew no longer strips symbols. " \ + "Pass explicit paths to prevent Homebrew from removing empty folders." end - find_method_with_args(body_node, :skip_clean, :all) do - problem <<~EOS.chomp - `skip_clean :all` is deprecated; brew no longer strips symbols - Pass explicit paths to prevent Homebrew from removing empty folders. - EOS + if find_method_def(@processed_source.ast) + problem "Define method #{method_name(@offensive_node)} in the class body, not at the top-level" end find_instance_method_call(body_node, :build, :universal?) do @@ -128,11 +317,65 @@ module RuboCop find_instance_method_call(body_node, "ENV", :x11) do problem 'Use "depends_on :x11" instead of "ENV.x11"' end + + find_every_method_call_by_name(body_node, :depends_on).each do |method| + next unless method_called?(method, :new) + problem "`depends_on` can take requirement classes instead of instances" + end + + os = [:leopard?, :snow_leopard?, :lion?, :mountain_lion?] + os.each do |version| + find_instance_method_call(body_node, "MacOS", version) do |method| + problem "\"#{method.source}\" is deprecated, use a comparison to MacOS.version instead" + end + end + + find_instance_method_call(body_node, "Dir", :[]) do |method| + path = parameters(method).first + next unless path.str_type? + next unless match = regex_match_group(path, /^[^\*{},]+$/) + problem "Dir([\"#{string_content(path)}\"]) is unnecessary; just use \"#{match[0]}\"" + end + + fileutils_methods = Regexp.new(FileUtils.singleton_methods(false).map { |m| "(?-mix:^" + Regexp.escape(m) + "$)" }.join("|")) + find_every_method_call_by_name(body_node, :system).each do |method| + param = parameters(method).first + next unless match = regex_match_group(param, fileutils_methods) + problem "Use the `#{match}` Ruby method instead of `#{method.source}`" + end + end + + def modifier?(node) + return false unless node.if_type? + node.modifier_form? end + def_node_search :conditional_dependencies, <<~EOS + {$(if (send (send nil? :build) ${:include? :with? :without?} $(str _)) + (send nil? :depends_on $({str sym} _)) nil?) + + $(if (send (send nil? :build) ${:include? :with? :without?} $(str _)) nil? + (send nil? :depends_on $({str sym} _)))} + EOS + + # Match depends_on with hash as argument + def_node_matcher :hash_dep, <<~EOS + {(hash (pair $(str _) $(str _))) + (hash (pair $(str _) (array $(str _) ...)))} + EOS + + def_node_matcher :destructure_hash, <<~EOS + (hash (pair $(str _) $(sym _))) + EOS + + def_node_search :formula_path_strings, <<~EOS + {(dstr (begin (send nil? %1)) $(str _ )) + (dstr _ (begin (send nil? %1)) $(str _ ))} + EOS + # Node Pattern search for Language::Node def_node_search :languageNodeModule?, <<~EOS - (const (const nil :Language) :Node) + (const (const nil? :Language) :Node) EOS end end diff --git a/Library/Homebrew/test/Gemfile.lock b/Library/Homebrew/test/Gemfile.lock index ba12d091d..1620d7099 100644 --- a/Library/Homebrew/test/Gemfile.lock +++ b/Library/Homebrew/test/Gemfile.lock @@ -36,7 +36,7 @@ GEM rspec-support (3.6.0) rspec-wait (0.0.9) rspec (>= 3, < 4) - rubocop (0.50.0) + rubocop (0.51.0) parallel (~> 1.10) parser (>= 2.3.3.1, < 3.0) powerpack (~> 0.1) @@ -61,7 +61,7 @@ DEPENDENCIES rspec rspec-its rspec-wait - rubocop (= 0.50.0) + rubocop (= 0.51.0) simplecov BUNDLED WITH diff --git a/Library/Homebrew/test/rubocops/bottle_block_cop_spec.rb b/Library/Homebrew/test/rubocops/bottle_block_cop_spec.rb index 659750858..e0982dcba 100644 --- a/Library/Homebrew/test/rubocops/bottle_block_cop_spec.rb +++ b/Library/Homebrew/test/rubocops/bottle_block_cop_spec.rb @@ -1,6 +1,3 @@ -require "rubocop" -require "rubocop/rspec/support" -require_relative "../../extend/string" require_relative "../../rubocops/bottle_block_cop" describe RuboCop::Cop::FormulaAuditStrict::BottleBlock do @@ -8,34 +5,16 @@ describe RuboCop::Cop::FormulaAuditStrict::BottleBlock do context "When auditing Bottle Block" do it "When there is revision in bottle block" do - source = <<~EOS + expect_offense(<<~RUBY) class Foo < Formula url 'http://example.com/foo-1.0.tgz' bottle do cellar :any revision 2 + ^^^^^^^^^^ Use rebuild instead of revision in bottle block end end - EOS - - expected_offenses = [{ message: described_class::MSG, - severity: :convention, - line: 5, - column: 4, - source: source }] - - inspect_source(source) - - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end - end - - def expect_offense(expected, actual) - expect(actual.message).to eq(expected[:message]) - expect(actual.severity).to eq(expected[:severity]) - expect(actual.line).to eq(expected[:line]) - expect(actual.column).to eq(expected[:column]) + RUBY end end @@ -50,6 +29,7 @@ describe RuboCop::Cop::FormulaAuditStrict::BottleBlock do end end EOS + corrected_source = <<~EOS class Foo < Formula url 'http://example.com/foo-1.0.tgz' diff --git a/Library/Homebrew/test/rubocops/caveats_cop_spec.rb b/Library/Homebrew/test/rubocops/caveats_cop_spec.rb index 68f79e08a..9c92d8fb8 100644 --- a/Library/Homebrew/test/rubocops/caveats_cop_spec.rb +++ b/Library/Homebrew/test/rubocops/caveats_cop_spec.rb @@ -1,6 +1,3 @@ -require "rubocop" -require "rubocop/rspec/support" -require_relative "../../extend/string" require_relative "../../rubocops/caveats_cop" describe RuboCop::Cop::FormulaAudit::Caveats do @@ -8,34 +5,16 @@ describe RuboCop::Cop::FormulaAudit::Caveats do context "When auditing caveats" do it "When there is setuid mentioned in caveats" do - source = <<~EOS + expect_offense(<<~RUBY) class Foo < Formula homepage "http://example.com/foo" url "http://example.com/foo-1.0.tgz" def caveats "setuid" + ^^^^^^ Don\'t recommend setuid in the caveats, suggest sudo instead. end end - EOS - - expected_offenses = [{ message: "Don't recommend setuid in the caveats, suggest sudo instead.", - severity: :convention, - line: 5, - column: 5, - source: source }] - - inspect_source(source) - - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end - end - - def expect_offense(expected, actual) - expect(actual.message).to eq(expected[:message]) - expect(actual.severity).to eq(expected[:severity]) - expect(actual.line).to eq(expected[:line]) - expect(actual.column).to eq(expected[:column]) + RUBY end end end diff --git a/Library/Homebrew/test/rubocops/checksum_cop_spec.rb b/Library/Homebrew/test/rubocops/checksum_cop_spec.rb index ab70f2dcf..3f720c90f 100644 --- a/Library/Homebrew/test/rubocops/checksum_cop_spec.rb +++ b/Library/Homebrew/test/rubocops/checksum_cop_spec.rb @@ -1,6 +1,3 @@ -require "rubocop" -require "rubocop/rspec/support" -require_relative "../../extend/string" require_relative "../../rubocops/checksum_cop" describe RuboCop::Cop::FormulaAudit::Checksum do @@ -8,105 +5,60 @@ describe RuboCop::Cop::FormulaAudit::Checksum do context "When auditing spec checksums" do it "When the checksum is empty" do - source = <<~EOS + expect_offense(<<~RUBY) class Foo < Formula url 'http://example.com/foo-1.0.tgz' stable do url "https://github.com/foo-lang/foo-compiler/archive/0.18.0.tar.gz" sha256 "" + ^^ sha256 is empty resource "foo-package" do url "https://github.com/foo-lang/foo-package/archive/0.18.0.tar.gz" sha256 "" + ^^ sha256 is empty end end end - EOS - - expected_offenses = [{ message: "sha256 is empty", - severity: :convention, - line: 5, - column: 12, - source: source }, - { message: "sha256 is empty", - severity: :convention, - line: 9, - column: 14, - source: source }] - - inspect_source(source) - - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + RUBY end it "When the checksum is not 64 characters" do - source = <<~EOS + expect_offense(<<~RUBY) class Foo < Formula url 'http://example.com/foo-1.0.tgz' stable do url "https://github.com/foo-lang/foo-compiler/archive/0.18.0.tar.gz" sha256 "5cf6e1ae0a645b426c0474cc7cd3f7d1605ffa1ac5756a39a8b2268ddc7ea0e9ad" + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ sha256 should be 64 characters resource "foo-package" do url "https://github.com/foo-lang/foo-package/archive/0.18.0.tar.gz" sha256 "5cf6e1ae0a645b426c047aaa4cc7cd3f7d1605ffa1ac5756a39a8b2268ddc7ea0e9" + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ sha256 should be 64 characters end end end - EOS - - expected_offenses = [{ message: "sha256 should be 64 characters", - severity: :convention, - line: 5, - column: 12, - source: source }, - { message: "sha256 should be 64 characters", - severity: :convention, - line: 9, - column: 14, - source: source }] - - inspect_source(source) - - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + RUBY end it "When the checksum has invalid chars" do - source = <<~EOS + expect_offense(<<~RUBY) class Foo < Formula url 'http://example.com/foo-1.0.tgz' stable do url "https://github.com/foo-lang/foo-compiler/archive/0.18.0.tar.gz" sha256 "5cf6e1ae0a645b426c0k7cc7cd3f7d1605ffa1ac5756a39a8b2268ddc7ea0e9a" + ^ sha256 contains invalid characters resource "foo-package" do url "https://github.com/foo-lang/foo-package/archive/0.18.0.tar.gz" sha256 "5cf6e1ae0a645b426x047aa4cc7cd3f7d1605ffa1ac5756a39a8b2268ddc7ea9" + ^ sha256 contains invalid characters end end end - EOS - - expected_offenses = [{ message: "sha256 contains invalid characters", - severity: :convention, - line: 5, - column: 31, - source: source }, - { message: "sha256 contains invalid characters", - severity: :convention, - line: 9, - column: 31, - source: source }] - - inspect_source(source) - - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + RUBY end end end @@ -116,46 +68,32 @@ describe RuboCop::Cop::FormulaAudit::ChecksumCase do context "When auditing spec checksums" do it "When the checksum has upper case characters" do - source = <<~EOS + expect_offense(<<~RUBY) class Foo < Formula url 'http://example.com/foo-1.0.tgz' stable do url "https://github.com/foo-lang/foo-compiler/archive/0.18.0.tar.gz" sha256 "5cf6e1ae0A645b426c0a7cc7cd3f7d1605ffa1ac5756a39a8b2268ddc7ea0e9a" + ^ sha256 should be lowercase resource "foo-package" do url "https://github.com/foo-lang/foo-package/archive/0.18.0.tar.gz" sha256 "5cf6e1Ae0a645b426b047aa4cc7cd3f7d1605ffa1ac5756a39a8b2268ddc7ea9" + ^ sha256 should be lowercase end end end - EOS - - expected_offenses = [{ message: "sha256 should be lowercase", - severity: :convention, - line: 5, - column: 21, - source: source }, - { message: "sha256 should be lowercase", - severity: :convention, - line: 9, - column: 20, - source: source }] - - inspect_source(source) - - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + RUBY end it "When auditing stable blocks outside spec blocks" do - source = <<~EOS + expect_offense(<<~RUBY) class Foo < Formula url 'http://example.com/foo-1.0.tgz' resource "foo-outside" do url "https://github.com/foo-lang/foo-outside/archive/0.18.0.tar.gz" sha256 "A4cc7cd3f7d1605ffa1ac5755cf6e1ae0a645b426b047a6a39a8b2268ddc7ea9" + ^ sha256 should be lowercase end stable do url "https://github.com/foo-lang/foo-compiler/archive/0.18.0.tar.gz" @@ -167,19 +105,7 @@ describe RuboCop::Cop::FormulaAudit::ChecksumCase do end end end - EOS - - expected_offenses = [{ message: "sha256 should be lowercase", - severity: :convention, - line: 5, - column: 12, - source: source }] - - inspect_source(source) - - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + RUBY end end diff --git a/Library/Homebrew/test/rubocops/class_cop_spec.rb b/Library/Homebrew/test/rubocops/class_cop_spec.rb index 3f210af11..2ea58777a 100644 --- a/Library/Homebrew/test/rubocops/class_cop_spec.rb +++ b/Library/Homebrew/test/rubocops/class_cop_spec.rb @@ -1,81 +1,62 @@ -require "rubocop" -require "rubocop/rspec/support" -require_relative "../../extend/string" require_relative "../../rubocops/class_cop" describe RuboCop::Cop::FormulaAudit::ClassName do subject(:cop) { described_class.new } - context "When auditing formula" do - it "with deprecated inheritance" do - formulas = [{ - "class" => "GithubGistFormula", - }, { - "class" => "ScriptFileFormula", - }, { - "class" => "AmazonWebServicesFormula", - }] - - formulas.each do |formula| - source = <<~EOS - class Foo < #{formula["class"]} - url 'http://example.com/foo-1.0.tgz' - end - EOS + it "reports an offense when using ScriptFileFormula" do + expect_offense(<<~RUBY) + class Foo < ScriptFileFormula + ^^^^^^^^^^^^^^^^^ ScriptFileFormula is deprecated, use Formula instead + url 'http://example.com/foo-1.0.tgz' + end + RUBY + end - expected_offenses = [{ message: "#{formula["class"]} is deprecated, use Formula instead", - severity: :convention, - line: 1, - column: 12, - source: source }] + it "reports an offense when using GithubGistFormula" do + expect_offense(<<~RUBY) + class Foo < GithubGistFormula + ^^^^^^^^^^^^^^^^^ GithubGistFormula is deprecated, use Formula instead + url 'http://example.com/foo-1.0.tgz' + end + RUBY + end - inspect_source(source) + it "reports an offense when using AmazonWebServicesFormula" do + expect_offense(<<~RUBY) + class Foo < AmazonWebServicesFormula + ^^^^^^^^^^^^^^^^^^^^^^^^ AmazonWebServicesFormula is deprecated, use Formula instead + url 'http://example.com/foo-1.0.tgz' + end + RUBY + end - expected_offenses.zip(cop.offenses.reverse).each do |expected, actual| - expect_offense(expected, actual) - end + it "supports auto-correcting deprecated parent classes" do + source = <<~EOS + class Foo < AmazonWebServicesFormula + url 'http://example.com/foo-1.0.tgz' end - end + EOS - it "with deprecated inheritance and autocorrect" do - source = <<~EOS - class Foo < AmazonWebServicesFormula - url 'http://example.com/foo-1.0.tgz' - end - EOS - corrected_source = <<~EOS - class Foo < Formula - url 'http://example.com/foo-1.0.tgz' - end - EOS + corrected_source = <<~EOS + class Foo < Formula + url 'http://example.com/foo-1.0.tgz' + end + EOS - new_source = autocorrect_source(source) - expect(new_source).to eq(corrected_source) - end + new_source = autocorrect_source(source) + expect(new_source).to eq(corrected_source) end end describe RuboCop::Cop::FormulaAuditStrict::Test do subject(:cop) { described_class.new } - context "When auditing formula" do - it "without a test block" do - source = <<~EOS - class Foo < Formula - url 'http://example.com/foo-1.0.tgz' - end - EOS - expected_offenses = [{ message: described_class::MSG, - severity: :convention, - line: 1, - column: 0, - source: source }] - - inspect_source(source) - - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) + it "reports an offense when there is no test block" do + expect_offense(<<~RUBY) + class Foo < Formula + ^^^^^^^^^^^^^^^^^^^ A `test do` test block should be added + url 'http://example.com/foo-1.0.tgz' end - end + RUBY end end diff --git a/Library/Homebrew/test/rubocops/components_order_cop_spec.rb b/Library/Homebrew/test/rubocops/components_order_cop_spec.rb index a4726c001..cd7cc5893 100644 --- a/Library/Homebrew/test/rubocops/components_order_cop_spec.rb +++ b/Library/Homebrew/test/rubocops/components_order_cop_spec.rb @@ -1,6 +1,3 @@ -require "rubocop" -require "rubocop/rspec/support" -require_relative "../../extend/string" require_relative "../../rubocops/components_order_cop" describe RuboCop::Cop::FormulaAuditStrict::ComponentsOrder do @@ -8,28 +5,17 @@ describe RuboCop::Cop::FormulaAuditStrict::ComponentsOrder do context "When auditing formula components order" do it "When url precedes homepage" do - source = <<~EOS + expect_offense(<<~RUBY) class Foo < Formula url "http://example.com/foo-1.0.tgz" homepage "http://example.com" + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `homepage` (line 3) should be put before `url` (line 2) end - EOS - - expected_offenses = [{ message: "`homepage` (line 3) should be put before `url` (line 2)", - severity: :convention, - line: 3, - column: 2, - source: source }] - - inspect_source(source) - - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + RUBY end it "When `resource` precedes `depends_on`" do - source = <<~EOS + expect_offense(<<~RUBY) class Foo < Formula url "https://example.com/foo-1.0.tgz" @@ -38,24 +24,13 @@ describe RuboCop::Cop::FormulaAuditStrict::ComponentsOrder do end depends_on "openssl" + ^^^^^^^^^^^^^^^^^^^^ `depends_on` (line 8) should be put before `resource` (line 4) end - EOS - - expected_offenses = [{ message: "`depends_on` (line 8) should be put before `resource` (line 4)", - severity: :convention, - line: 8, - column: 2, - source: source }] - - inspect_source(source) - - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + RUBY end it "When `test` precedes `plist`" do - source = <<~EOS + expect_offense(<<~RUBY) class Foo < Formula url "https://example.com/foo-1.0.tgz" @@ -64,53 +39,24 @@ describe RuboCop::Cop::FormulaAuditStrict::ComponentsOrder do end def plist + ^^^^^^^^^ `plist` (line 8) should be put before `test` (line 4) end end - EOS - - expected_offenses = [{ message: "`plist` (line 8) should be put before `test` (line 4)", - severity: :convention, - line: 8, - column: 2, - source: source }] - - inspect_source(source) - - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + RUBY end it "When only one of many `depends_on` precedes `conflicts_with`" do - source = <<~EOS + expect_offense(<<~RUBY) class Foo < Formula depends_on "autoconf" => :build conflicts_with "visionmedia-watch" depends_on "automake" => :build + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `depends_on` (line 4) should be put before `conflicts_with` (line 3) depends_on "libtool" => :build depends_on "pkg-config" => :build depends_on "gettext" end - EOS - - expected_offenses = [{ message: "`depends_on` (line 4) should be put before `conflicts_with` (line 3)", - severity: :convention, - line: 4, - column: 2, - source: source }] - - inspect_source(source) - - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end - end - - def expect_offense(expected, actual) - expect(actual.message).to eq(expected[:message]) - expect(actual.severity).to eq(expected[:severity]) - expect(actual.line).to eq(expected[:line]) - expect(actual.column).to eq(expected[:column]) + RUBY end end @@ -122,6 +68,7 @@ describe RuboCop::Cop::FormulaAuditStrict::ComponentsOrder do homepage "http://example.com" end EOS + correct_source = <<~EOS class Foo < Formula homepage "http://example.com" @@ -145,6 +92,7 @@ describe RuboCop::Cop::FormulaAuditStrict::ComponentsOrder do depends_on "openssl" end EOS + correct_source = <<~EOS class Foo < Formula url "https://example.com/foo-1.0.tgz" @@ -156,6 +104,7 @@ describe RuboCop::Cop::FormulaAuditStrict::ComponentsOrder do end end EOS + corrected_source = autocorrect_source(source) expect(corrected_source).to eq(correct_source) end diff --git a/Library/Homebrew/test/rubocops/components_redundancy_cop_spec.rb b/Library/Homebrew/test/rubocops/components_redundancy_cop_spec.rb index e899a9b07..d8878ae79 100644 --- a/Library/Homebrew/test/rubocops/components_redundancy_cop_spec.rb +++ b/Library/Homebrew/test/rubocops/components_redundancy_cop_spec.rb @@ -1,6 +1,3 @@ -require "rubocop" -require "rubocop/rspec/support" -require_relative "../../extend/string" require_relative "../../rubocops/components_redundancy_cop" describe RuboCop::Cop::FormulaAuditStrict::ComponentsRedundancy do @@ -8,80 +5,40 @@ describe RuboCop::Cop::FormulaAuditStrict::ComponentsRedundancy do context "When auditing formula components common errors" do it "When url outside stable block" do - source = <<~EOS + expect_offense(<<~RUBY) class Foo < Formula url "http://example.com/foo-1.0.tgz" + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `url` should be put inside `stable` block stable do # stuff end end - EOS - - expected_offenses = [{ message: "`url` should be put inside `stable` block", - severity: :convention, - line: 2, - column: 2, - source: source }] - - inspect_source(source) - - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + RUBY end it "When both `head` and `head do` are present" do - source = <<~EOS + expect_offense(<<~RUBY) class Foo < Formula head "http://example.com/foo.git" head do + ^^^^^^^ `head` and `head do` should not be simultaneously present # stuff end end - EOS - - expected_offenses = [{ message: "`head` and `head do` should not be simultaneously present", - severity: :convention, - line: 3, - column: 2, - source: source }] - - inspect_source(source) - - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + RUBY end it "When both `bottle :modifier` and `bottle do` are present" do - source = <<~EOS + expect_offense(<<~RUBY) class Foo < Formula url "http://example.com/foo-1.0.tgz" bottle do + ^^^^^^^^^ `bottle :modifier` and `bottle do` should not be simultaneously present # bottles go here end bottle :unneeded end - EOS - - expected_offenses = [{ message: "`bottle :modifier` and `bottle do` should not be simultaneously present", - severity: :convention, - line: 3, - column: 2, - source: source }] - - inspect_source(source) - - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end - end - - def expect_offense(expected, actual) - expect(actual.message).to eq(expected[:message]) - expect(actual.severity).to eq(expected[:severity]) - expect(actual.line).to eq(expected[:line]) - expect(actual.column).to eq(expected[:column]) + RUBY end end end diff --git a/Library/Homebrew/test/rubocops/conflicts_cop_spec.rb b/Library/Homebrew/test/rubocops/conflicts_cop_spec.rb index 40efe8545..5f6f020fd 100644 --- a/Library/Homebrew/test/rubocops/conflicts_cop_spec.rb +++ b/Library/Homebrew/test/rubocops/conflicts_cop_spec.rb @@ -1,6 +1,3 @@ -require "rubocop" -require "rubocop/rspec/support" -require_relative "../../extend/string" require_relative "../../rubocops/conflicts_cop" describe RuboCop::Cop::FormulaAudit::Conflicts do @@ -8,36 +5,23 @@ describe RuboCop::Cop::FormulaAudit::Conflicts do context "When auditing formula for conflicts with" do it "multiple conflicts_with" do - source = <<~EOS + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo@2.0.rb") class FooAT20 < Formula url 'http://example.com/foo-2.0.tgz' conflicts_with "mysql", "mariadb", "percona-server", + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Versioned formulae should not use `conflicts_with`. Use `keg_only :versioned_formula` instead. :because => "both install plugins" end - EOS - - expected_offenses = [{ message: described_class::MSG, - severity: :convention, - line: 3, - column: 2, - source: source }] - - inspect_source(source, "/homebrew-core/Formula/foo@2.0.rb") - - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + RUBY end it "no conflicts_with" do - source = <<~EOS + expect_no_offenses(<<~RUBY, "/homebrew-core/Formula/foo@2.0.rb") class FooAT20 < Formula url 'http://example.com/foo-2.0.tgz' desc 'Bar' end - EOS - inspect_source(source, "/homebrew-core/Formula/foo@2.0.rb") - expect(cop.offenses).to eq([]) + RUBY end end end diff --git a/Library/Homebrew/test/rubocops/formula_desc_cop_spec.rb b/Library/Homebrew/test/rubocops/formula_desc_cop_spec.rb index 0f0189aa8..3ddc42d34 100644 --- a/Library/Homebrew/test/rubocops/formula_desc_cop_spec.rb +++ b/Library/Homebrew/test/rubocops/formula_desc_cop_spec.rb @@ -1,6 +1,3 @@ -require "rubocop" -require "rubocop/rspec/support" -require_relative "../../extend/string" require_relative "../../rubocops/formula_desc_cop" describe RuboCop::Cop::FormulaAuditStrict::DescLength do @@ -8,93 +5,43 @@ describe RuboCop::Cop::FormulaAuditStrict::DescLength do context "When auditing formula desc" do it "When there is no desc" do - source = <<~EOS + expect_offense(<<~RUBY) class Foo < Formula + ^^^^^^^^^^^^^^^^^^^ Formula should have a desc (Description). url 'http://example.com/foo-1.0.tgz' end - EOS - - expected_offenses = [{ message: "Formula should have a desc (Description).", - severity: :convention, - line: 1, - column: 0, - source: source }] - - inspect_source(source) - - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + RUBY end it "reports an offense when desc is an empty string" do - source = <<~EOS + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") class Foo < Formula url 'http://example.com/foo-1.0.tgz' desc '' + ^^^^^^^ The desc (description) should not be an empty string. end - EOS - - msg = "The desc (description) should not be an empty string." - expected_offenses = [{ message: msg, - severity: :convention, - line: 3, - column: 2, - source: source }] - - inspect_source(source, "/homebrew-core/Formula/foo.rb") - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + RUBY end it "When desc is too long" do - source = <<~EOS + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") class Foo < Formula url 'http://example.com/foo-1.0.tgz' desc 'Bar#{"bar" * 29}' + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Description is too long. "name: desc" should be less than 80 characters. Length is calculated as foo + desc. (currently 95) end - EOS - - msg = <<~EOS - Description is too long. "name: desc" should be less than 80 characters. - Length is calculated as foo + desc. (currently 95) - EOS - expected_offenses = [{ message: msg, - severity: :convention, - line: 3, - column: 2, - source: source }] - - inspect_source(source, "/homebrew-core/Formula/foo.rb") - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + RUBY end it "When desc is multiline string" do - source = <<~EOS + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") class Foo < Formula url 'http://example.com/foo-1.0.tgz' desc 'Bar#{"bar" * 9}'\ '#{"foo" * 21}' + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Description is too long. "name: desc" should be less than 80 characters. Length is calculated as foo + desc. (currently 98) end - EOS - - msg = <<~EOS - Description is too long. "name: desc" should be less than 80 characters. - Length is calculated as foo + desc. (currently 98) - EOS - expected_offenses = [{ message: msg, - severity: :convention, - line: 3, - column: 2, - source: source }] - - inspect_source(source, "/homebrew-core/Formula/foo.rb") - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + RUBY end end end @@ -104,83 +51,44 @@ describe RuboCop::Cop::FormulaAuditStrict::Desc do context "When auditing formula desc" do it "When wrong \"command-line\" usage in desc" do - source = <<~EOS + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") class Foo < Formula url 'http://example.com/foo-1.0.tgz' desc 'command line' + ^ Description should start with a capital letter + ^^^^^^^^^^^^ Description should use \"command-line\" instead of \"command line\" end - EOS - - expected_offenses = [{ message: "Description should use \"command-line\" instead of \"command line\"", - severity: :convention, - line: 3, - column: 8, - source: source }] - - inspect_source(source) - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + RUBY end it "When an article is used in desc" do - source = <<~EOS + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") class Foo < Formula url 'http://example.com/foo-1.0.tgz' desc 'An ' + ^^^ Description shouldn\'t start with an indefinite article i.e. \"An\" end - EOS - - expected_offenses = [{ message: "Description shouldn't start with an indefinite article i.e. \"An\"", - severity: :convention, - line: 3, - column: 8, - source: source }] - - inspect_source(source) - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + RUBY end it "When an lowercase letter starts a desc" do - source = <<~EOS + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") class Foo < Formula url 'http://example.com/foo-1.0.tgz' desc 'bar' + ^ Description should start with a capital letter end - EOS - - expected_offenses = [{ message: "Description should start with a capital letter", - severity: :convention, - line: 3, - column: 8, - source: source }] - - inspect_source(source) - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + RUBY end it "When formula name is in desc" do - source = <<~EOS + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") class Foo < Formula url 'http://example.com/foo-1.0.tgz' desc 'Foo is a foobar' + ^^^^ Description shouldn\'t start with the formula name end - EOS - - expected_offenses = [{ message: "Description shouldn't start with the formula name", - severity: :convention, - line: 3, - column: 8, - source: source }] - - inspect_source(source, "/homebrew-core/Formula/foo.rb") - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + RUBY end it "autocorrects all rules" do @@ -190,6 +98,7 @@ describe RuboCop::Cop::FormulaAuditStrict::Desc do desc ' an bar: commandline foo ' end EOS + correct_source = <<~EOS class Foo < Formula url 'http://example.com/foo-1.0.tgz' diff --git a/Library/Homebrew/test/rubocops/homepage_cop_spec.rb b/Library/Homebrew/test/rubocops/homepage_cop_spec.rb index be9dddae6..d394f94a1 100644 --- a/Library/Homebrew/test/rubocops/homepage_cop_spec.rb +++ b/Library/Homebrew/test/rubocops/homepage_cop_spec.rb @@ -1,6 +1,3 @@ -require "rubocop" -require "rubocop/rspec/support" -require_relative "../../extend/string" require_relative "../../rubocops/homepage_cop" describe RuboCop::Cop::FormulaAudit::Homepage do diff --git a/Library/Homebrew/test/rubocops/lines_cop_spec.rb b/Library/Homebrew/test/rubocops/lines_cop_spec.rb index 7e93bee75..e65eff1fc 100644 --- a/Library/Homebrew/test/rubocops/lines_cop_spec.rb +++ b/Library/Homebrew/test/rubocops/lines_cop_spec.rb @@ -1,260 +1,342 @@ -require "rubocop" -require "rubocop/rspec/support" -require_relative "../../extend/string" require_relative "../../rubocops/lines_cop" describe RuboCop::Cop::FormulaAudit::Lines do subject(:cop) { described_class.new } - context "When auditing lines" do - it "with correctable deprecated dependencies" do - formulae = [{ - "dependency" => :automake, - "correct" => "automake", - }, { - "dependency" => :autoconf, - "correct" => "autoconf", - }, { - "dependency" => :libtool, - "correct" => "libtool", - }, { - "dependency" => :apr, - "correct" => "apr-util", - }, { - "dependency" => :tex, - }] - - formulae.each do |formula| - source = <<~EOS - class Foo < Formula - url 'http://example.com/foo-1.0.tgz' - depends_on :#{formula["dependency"]} - end - EOS - if formula.key?("correct") - offense = ":#{formula["dependency"]} is deprecated. Usage should be \"#{formula["correct"]}\"" - else - offense = ":#{formula["dependency"]} is deprecated" - end - expected_offenses = [{ message: offense, - severity: :convention, - line: 3, - column: 2, - source: source }] + it "reports an offense when using depends_on :automake" do + expect_offense(<<~RUBY) + class Foo < Formula + url 'http://example.com/foo-1.0.tgz' + depends_on :automake + ^^^^^^^^^^^^^^^^^^^^ :automake is deprecated. Usage should be \"automake\". + end + RUBY + end - inspect_source(source) + it "reports an offense when using depends_on :autoconf" do + expect_offense(<<~RUBY) + class Foo < Formula + url 'http://example.com/foo-1.0.tgz' + depends_on :autoconf + ^^^^^^^^^^^^^^^^^^^^ :autoconf is deprecated. Usage should be \"autoconf\". + end + RUBY + end - expected_offenses.zip(cop.offenses.reverse).each do |expected, actual| - expect_offense(expected, actual) - end + it "reports an offense when using depends_on :libtool" do + expect_offense(<<~RUBY) + class Foo < Formula + url 'http://example.com/foo-1.0.tgz' + depends_on :libtool + ^^^^^^^^^^^^^^^^^^^ :libtool is deprecated. Usage should be \"libtool\". end - end + RUBY + end + + it "reports an offense when using depends_on :apr" do + expect_offense(<<~RUBY) + class Foo < Formula + url 'http://example.com/foo-1.0.tgz' + depends_on :apr + ^^^^^^^^^^^^^^^ :apr is deprecated. Usage should be \"apr-util\". + end + RUBY + end + + it "reports an offense when using depends_on :tex" do + expect_offense(<<~RUBY) + class Foo < Formula + url 'http://example.com/foo-1.0.tgz' + depends_on :tex + ^^^^^^^^^^^^^^^ :tex is deprecated. + end + RUBY end end describe RuboCop::Cop::FormulaAudit::ClassInheritance do subject(:cop) { described_class.new } - context "When auditing lines" do - it "with no space in class inheritance" do - source = <<~EOS - class Foo<Formula - desc "foo" - url 'http://example.com/foo-1.0.tgz' - end - EOS - - expected_offenses = [{ message: "Use a space in class inheritance: class Foo < Formula", - severity: :convention, - line: 1, - column: 10, - source: source }] - - inspect_source(source) - - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) + it "reports an offense when not using spaces for class inheritance" do + expect_offense(<<~RUBY, "/homebrew-core/Formula/foo.rb") + class Foo<Formula + ^^^^^^^ Use a space in class inheritance: class Foo < Formula + desc "foo" + url 'http://example.com/foo-1.0.tgz' end - end + RUBY end end describe RuboCop::Cop::FormulaAudit::Comments do subject(:cop) { described_class.new } - context "When auditing formulae" do - it "with commented cmake call" do - source = <<~EOS + context "When auditing formula" do + it "commented cmake call" do + expect_offense(<<~RUBY) class Foo < Formula desc "foo" url 'http://example.com/foo-1.0.tgz' # system "cmake", ".", *std_cmake_args + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Please remove default template comments end - EOS - - expected_offenses = [{ message: "Please remove default template comments", - severity: :convention, - line: 4, - column: 2, - source: source }] - - inspect_source(source) - - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + RUBY end - it "with default template comments" do - source = <<~EOS + it "default template comments" do + expect_offense(<<~RUBY) class Foo < Formula # PLEASE REMOVE + ^^^^^^^^^^^^^^^ Please remove default template comments desc "foo" url 'http://example.com/foo-1.0.tgz' end - EOS - - expected_offenses = [{ message: "Please remove default template comments", - severity: :convention, - line: 2, - column: 2, - source: source }] - - inspect_source(source) - - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + RUBY end - it "with commented out depends_on" do - source = <<~EOS + it "commented out depends_on" do + expect_offense(<<~RUBY) class Foo < Formula desc "foo" url 'http://example.com/foo-1.0.tgz' # depends_on "foo" + ^^^^^^^^^^^^^^^^^^ Commented-out dependency "foo" end - EOS + RUBY + end + end +end - expected_offenses = [{ message: 'Commented-out dependency "foo"', - severity: :convention, - line: 4, - column: 2, - source: source }] +describe RuboCop::Cop::FormulaAudit::AssertStatements do + subject(:cop) { described_class.new } - inspect_source(source) + it "assert ...include usage" do + expect_offense(<<~RUBY) + class Foo < Formula + desc "foo" + url 'http://example.com/foo-1.0.tgz' + assert File.read("inbox").include?("Sample message 1") + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `assert_match` instead of `assert ...include?` + end + RUBY + end - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) + it "assert ...exist? without a negation" do + expect_offense(<<~RUBY) + class Foo < Formula + desc "foo" + url 'http://example.com/foo-1.0.tgz' + assert File.exist? "default.ini" + ^^^^^^^^^^^^^^^^^^^^^^^^^ Use `assert_predicate <path_to_file>, :exist?` instead of `assert File.exist? "default.ini"` end - end + RUBY + end + + it "assert ...exist? with a negation" do + expect_offense(<<~RUBY) + class Foo < Formula + desc "foo" + url 'http://example.com/foo-1.0.tgz' + assert !File.exist?("default.ini") + ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `refute_predicate <path_to_file>, :exist?` instead of `assert !File.exist?("default.ini")` + end + RUBY + end + + it "assert ...executable? without a negation" do + expect_offense(<<~RUBY) + class Foo < Formula + desc "foo" + url 'http://example.com/foo-1.0.tgz' + assert File.executable? f + ^^^^^^^^^^^^^^^^^^ Use `assert_predicate <path_to_file>, :executable?` instead of `assert File.executable? f` + end + RUBY + end +end + +describe RuboCop::Cop::FormulaAudit::OptionDeclarations do + subject(:cop) { described_class.new } + + it "unless build.without? conditional" do + expect_offense(<<~RUBY) + class Foo < Formula + desc "foo" + url 'http://example.com/foo-1.0.tgz' + def post_install + return unless build.without? "bar" + ^^^^^^^^^^^^^^^^^^^^ Use if build.with? "bar" instead of unless build.without? "bar" + end + end + RUBY + end + + it "unless build.with? conditional" do + expect_offense(<<~RUBY) + class Foo < Formula + desc "foo" + url 'http://example.com/foo-1.0.tgz' + def post_install + return unless build.with? "bar" + ^^^^^^^^^^^^^^^^^ Use if build.without? "bar" instead of unless build.with? "bar" + end + end + RUBY + end + + it "negated build.with? conditional" do + expect_offense(<<~RUBY) + class Foo < Formula + desc "foo" + url 'http://example.com/foo-1.0.tgz' + def post_install + return if !build.with? "bar" + ^^^^^^^^^^^^^^^^^^ Don't negate 'build.with?': use 'build.without?' + end + end + RUBY + end + + it "negated build.without? conditional" do + expect_offense(<<~RUBY) + class Foo < Formula + desc "foo" + url 'http://example.com/foo-1.0.tgz' + def post_install + return if !build.without? "bar" + ^^^^^^^^^^^^^^^^^^^^^ Don't negate 'build.without?': use 'build.with?' + end + end + RUBY + end + + it "unnecessary build.without? conditional" do + expect_offense(<<~RUBY) + class Foo < Formula + desc "foo" + url 'http://example.com/foo-1.0.tgz' + def post_install + return if build.without? "--without-bar" + ^^^^^^^^^^^^^ Don't duplicate 'without': Use `build.without? \"bar\"` to check for \"--without-bar\" + end + end + RUBY + end + + it "unnecessary build.with? conditional" do + expect_offense(<<~RUBY) + class Foo < Formula + desc "foo" + url 'http://example.com/foo-1.0.tgz' + def post_install + return if build.with? "--with-bar" + ^^^^^^^^^^ Don't duplicate 'with': Use `build.with? \"bar\"` to check for \"--with-bar\" + end + end + RUBY + end + + it "build.include? conditional" do + expect_offense(<<~RUBY) + class Foo < Formula + desc "foo" + url 'http://example.com/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 'http://example.com/foo-1.0.tgz' + def post_install + return if build.include? "--bar" + ^^^^^ Reference 'bar' without dashes + end + end + RUBY + end + + it "def options usage" do + expect_offense(<<~RUBY) + class Foo < Formula + desc "foo" + url 'http://example.com/foo-1.0.tgz' + + def options + ^^^^^^^^^^^ Use new-style option definitions + [["--bar", "desc"]] + end + end + RUBY end end describe RuboCop::Cop::FormulaAudit::Miscellaneous do subject(:cop) { described_class.new } - context "When auditing formulae" do - it "with FileUtils" do - source = <<~EOS + context "When auditing formula" do + it "FileUtils usage" do + expect_offense(<<~RUBY) class Foo < Formula desc "foo" url 'http://example.com/foo-1.0.tgz' FileUtils.mv "hello" + ^^^^^^^^^^^^^^^^^^^^ Don\'t need \'FileUtils.\' before mv end - EOS - - expected_offenses = [{ message: "Don't need 'FileUtils.' before mv", - severity: :convention, - line: 4, - column: 2, - source: source }] - - inspect_source(source) - - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + RUBY end - it "with long inreplace block vars" do - source = <<~EOS + it "long inreplace block vars" do + expect_offense(<<~RUBY) class Foo < Formula desc "foo" url 'http://example.com/foo-1.0.tgz' inreplace "foo" do |longvar| + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ \"inreplace <filenames> do |s|\" is preferred over \"|longvar|\". somerandomCall(longvar) end end - EOS - - expected_offenses = [{ message: "\"inreplace <filenames> do |s|\" is preferred over \"|longvar|\".", - severity: :convention, - line: 4, - column: 2, - source: source }] - - inspect_source(source) - - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + RUBY end - it "with invalid rebuild" do - source = <<~EOS + it "an invalid rebuild statement" do + expect_offense(<<~RUBY) class Foo < Formula desc "foo" url 'http://example.com/foo-1.0.tgz' bottle do rebuild 0 + ^^^^^^^^^ 'rebuild 0' should be removed sha256 "fe0679b932dd43a87fd415b609a7fbac7a069d117642ae8ebaac46ae1fb9f0b3" => :sierra end end - EOS - - expected_offenses = [{ message: "'rebuild 0' should be removed", - severity: :convention, - line: 5, - column: 4, - source: source }] - - inspect_source(source) - - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + RUBY end - it "with OS.linux? check" do - source = <<~EOS + it "OS.linux? check" do + expect_offense(<<~RUBY, "/homebrew-core/") class Foo < Formula desc "foo" url 'http://example.com/foo-1.0.tgz' bottle do if OS.linux? + ^^^^^^^^^ Don\'t use OS.linux?; Homebrew/core only supports macOS nil end sha256 "fe0679b932dd43a87fd415b609a7fbac7a069d117642ae8ebaac46ae1fb9f0b3" => :sierra end end - EOS - - expected_offenses = [{ message: "Don't use OS.linux?; Homebrew/core only supports macOS", - severity: :convention, - line: 5, - column: 7, - source: source }] - - inspect_source(source, "/homebrew-core/") - - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + RUBY end - it "with fails_with :llvm" do - source = <<~EOS + it "fails_with :llvm block" do + expect_offense(<<~RUBY) class Foo < Formula desc "foo" url 'http://example.com/foo-1.0.tgz' @@ -262,270 +344,450 @@ describe RuboCop::Cop::FormulaAudit::Miscellaneous do sha256 "fe0679b932dd43a87fd415b609a7fbac7a069d117642ae8ebaac46ae1fb9f0b3" => :sierra end fails_with :llvm do + ^^^^^^^^^^^^^^^^ 'fails_with :llvm' is now a no-op so should be removed build 2335 cause "foo" end end - EOS - - expected_offenses = [{ message: "'fails_with :llvm' is now a no-op so should be removed", - severity: :convention, - line: 7, - column: 2, - source: source }] - - inspect_source(source) - - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + RUBY end - it "with def test" do - source = <<~EOS + it "def test's deprecated usage" do + expect_offense(<<~RUBY) class Foo < Formula desc "foo" url 'http://example.com/foo-1.0.tgz' def test + ^^^^^^^^ Use new-style test definitions (test do) assert_equals "1", "1" end end - EOS + RUBY + end - expected_offenses = [{ message: "Use new-style test definitions (test do)", - severity: :convention, - line: 5, - column: 2, - source: source }] + it "with deprecated skip_clean call" do + expect_offense(<<~RUBY) + class Foo < Formula + desc "foo" + url 'http://example.com/foo-1.0.tgz' + skip_clean :all + ^^^^^^^^^^^^^^^ `skip_clean :all` is deprecated; brew no longer strips symbols. Pass explicit paths to prevent Homebrew from removing empty folders. + end + RUBY + end - inspect_source(source) + it "build.universal? deprecated usage" do + expect_offense(<<~RUBY) + class Foo < Formula + desc "foo" + url 'http://example.com/foo-1.0.tgz' + if build.universal? + ^^^^^^^^^^^^^^^^ macOS has been 64-bit only since 10.6 so build.universal? is deprecated. + "foo" + end + end + RUBY + end - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + it "build.universal? deprecation exempted formula" do + expect_no_offenses(<<~RUBY, "/homebrew-core/Formula/wine.rb") + class Wine < Formula + desc "foo" + url 'http://example.com/foo-1.0.tgz' + if build.universal? + "foo" + end + end + RUBY end - it "with def options" do - source = <<~EOS + it "deprecated ENV.universal_binary usage" do + expect_offense(<<~RUBY) class Foo < Formula desc "foo" url 'http://example.com/foo-1.0.tgz' + if build? + ENV.universal_binary + ^^^^^^^^^^^^^^^^^^^^ macOS has been 64-bit only since 10.6 so ENV.universal_binary is deprecated. + end + end + RUBY + end - def options - [["--bar", "desc"]] + it "ENV.universal_binary deprecation exempted formula" do + expect_no_offenses(<<~RUBY, "/homebrew-core/Formula/wine.rb") + class Wine < Formula + desc "foo" + url 'http://example.com/foo-1.0.tgz' + if build? + ENV.universal_binary end end - EOS + RUBY + end - expected_offenses = [{ message: "Use new-style option definitions", - severity: :convention, - line: 5, - column: 2, - source: source }] + it "deprecated ENV.x11 usage" do + expect_offense(<<~RUBY) + class Foo < Formula + desc "foo" + url 'http://example.com/foo-1.0.tgz' + if build? + ENV.x11 + ^^^^^^^ Use "depends_on :x11" instead of "ENV.x11" + end + end + RUBY + end - inspect_source(source) + it "install_name_tool usage instead of ruby-macho" do + expect_offense(<<~RUBY) + class Foo < Formula + desc "foo" + url 'http://example.com/foo-1.0.tgz' + system "install_name_tool", "-id" + ^^^^^^^^^^^^^^^^^ Use ruby-macho instead of calling "install_name_tool" + end + RUBY + end - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + it "ruby-macho alternatives audit exempted formula" do + expect_no_offenses(<<~RUBY, "/homebrew-core/Formula/cctools.rb") + class Cctools < Formula + desc "foo" + url 'http://example.com/foo-1.0.tgz' + system "install_name_tool", "-id" + end + RUBY end - it "with deprecated skip_clean call" do - source = <<~EOS + it "npm install without language::Node args" do + expect_offense(<<~RUBY) class Foo < Formula desc "foo" url 'http://example.com/foo-1.0.tgz' - skip_clean :all + system "npm", "install" + ^^^^^^^^^^^^^^^^^^^^^^^ Use Language::Node for npm install args end - EOS - - expected_offenses = [{ message: <<~EOS.chomp, - `skip_clean :all` is deprecated; brew no longer strips symbols - Pass explicit paths to prevent Homebrew from removing empty folders. - EOS - severity: :convention, - line: 4, - column: 2, - source: source }] + RUBY + end - inspect_source(source) + it "npm install without language::Node args in kibana(exempted formula)" do + expect_no_offenses(<<~RUBY, "/homebrew-core/Formula/kibana@4.4.rb") + class KibanaAT44 < Formula + desc "foo" + url 'http://example.com/foo-1.0.tgz' + system "npm", "install" + end + RUBY + end - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + it "depends_on with an instance as argument" do + expect_offense(<<~RUBY) + class Foo < Formula + desc "foo" + url 'http://example.com/foo-1.0.tgz' + depends_on FOO::BAR.new + ^^^^^^^^^^^^ `depends_on` can take requirement classes instead of instances + end + RUBY end - it "with build.universal?" do - source = <<~EOS + it "old style OS check" do + expect_offense(<<~RUBY) class Foo < Formula desc "foo" url 'http://example.com/foo-1.0.tgz' - if build.universal? - "foo" - end + depends_on :foo if MacOS.snow_leopard? + ^^^^^^^^^^^^^^^^^^^ \"MacOS.snow_leopard?\" is deprecated, use a comparison to MacOS.version instead end - EOS + RUBY + end - expected_offenses = [{ message: "macOS has been 64-bit only since 10.6 so build.universal? is deprecated.", - severity: :convention, - line: 4, - column: 5, - source: source }] + it "non glob DIR usage" do + expect_offense(<<~RUBY) + class Foo < Formula + desc "foo" + url 'http://example.com/foo-1.0.tgz' + rm_rf Dir["src/{llvm,test,librustdoc,etc/snapshot.pyc}"] + rm_rf Dir["src/snapshot.pyc"] + ^^^^^^^^^^^^^^^^ Dir(["src/snapshot.pyc"]) is unnecessary; just use "src/snapshot.pyc" + end + RUBY + end - inspect_source(source) + it "system call to fileUtils Method" do + expect_offense(<<~RUBY) + class Foo < Formula + desc "foo" + url 'http://example.com/foo-1.0.tgz' + system "mkdir", "foo" + ^^^^^ Use the `mkdir` Ruby method instead of `system "mkdir", "foo"` + end + RUBY + end - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + it "top-level function def outside class body" do + expect_offense(<<~RUBY) + def test + ^^^^^^^^ Define method test in the class body, not at the top-level + nil + end + class Foo < Formula + desc "foo" + url 'http://example.com/foo-1.0.tgz' + end + RUBY end - it "with a build.universal? exemption reports no offenses" do - source = <<~EOS - class Wine < Formula + it "Using ARGV to check options" do + expect_offense(<<~RUBY) + class Foo < Formula desc "foo" url 'http://example.com/foo-1.0.tgz' - if build.universal? - "foo" + def install + verbose = ARGV.verbose? + ^^^^^^^^^^^^^ Use build instead of ARGV to check options end end - EOS - - inspect_source(source, "/homebrew-core/Formula/wine.rb") - expect(cop.offenses).to be_empty + RUBY end - it "with ENV.universal_binary" do - source = <<~EOS + it 'man+"man8" usage' do + expect_offense(<<~RUBY) class Foo < Formula desc "foo" url 'http://example.com/foo-1.0.tgz' - if build? - ENV.universal_binary + def install + man1.install man+"man8" => "faad.1" + ^^^^ "man+"man8"" should be "man8" end end - EOS - - expected_offenses = [{ message: "macOS has been 64-bit only since 10.6 so ENV.universal_binary is deprecated.", - severity: :convention, - line: 5, - column: 5, - source: source }] - - inspect_source(source) - - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + RUBY end - it "with an ENV.universal_binary exemption reports no offenses" do - source = <<~EOS - class Wine < Formula + it "hardcoded gcc compiler" do + expect_offense(<<~'RUBY') + class Foo < Formula desc "foo" url 'http://example.com/foo-1.0.tgz' - if build? - ENV.universal_binary + def install + system "/usr/bin/gcc", "foo" + ^^^^^^^^^^^^ Use "#{ENV.cc}" instead of hard-coding "gcc" end end - EOS + RUBY + end - inspect_source(source, "/homebrew-core/Formula/wine.rb") - expect(cop.offenses).to be_empty + it "hardcoded g++ compiler" do + expect_offense(<<~'RUBY') + class Foo < Formula + desc "foo" + url 'http://example.com/foo-1.0.tgz' + def install + system "/usr/bin/g++", "-o", "foo", "foo.cc" + ^^^^^^^^^^^^ Use "#{ENV.cxx}" instead of hard-coding "g++" + end + end + RUBY end - it "with ENV.x11" do - source = <<~EOS + it "hardcoded llvm-g++ compiler" do + expect_offense(<<~'RUBY') class Foo < Formula desc "foo" url 'http://example.com/foo-1.0.tgz' - if build? - ENV.x11 + def install + ENV["COMPILER_PATH"] = "/usr/bin/llvm-g++" + ^^^^^^^^^^^^^^^^^ Use "#{ENV.cxx}" instead of hard-coding "llvm-g++" end end - EOS + RUBY + end - expected_offenses = [{ message: 'Use "depends_on :x11" instead of "ENV.x11"', - severity: :convention, - line: 5, - column: 5, - source: source }] + it "hardcoded gcc compiler" do + expect_offense(<<~RUBY) + class Foo < Formula + desc "foo" + url 'http://example.com/foo-1.0.tgz' + def install + ENV["COMPILER_PATH"] = "/usr/bin/gcc" + ^^^^^^^^^^^^ Use \"\#{ENV.cc}\" instead of hard-coding \"gcc\" + end + end + RUBY + end - inspect_source(source) + it "formula path shortcut : man" do + expect_offense(<<~'RUBY') + class Foo < Formula + desc "foo" + url 'http://example.com/foo-1.0.tgz' + def install + mv "#{share}/man", share + ^^^^ "#{share}/man" should be "#{man}" + end + end + RUBY + end - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + it "formula path shortcut : libexec" do + expect_offense(<<~'RUBY') + class Foo < Formula + desc "foo" + url 'http://example.com/foo-1.0.tgz' + def install + mv "#{prefix}/libexec", share + ^^^^^^^^ "#{prefix}/libexec" should be "#{libexec}" + end + end + RUBY end - it "with ruby-macho alternatives" do - source = <<~EOS + it "formula path shortcut : info" do + expect_offense(<<~'RUBY') class Foo < Formula desc "foo" url 'http://example.com/foo-1.0.tgz' - system "install_name_tool", "-id" + def install + system "./configure", "--INFODIR=#{prefix}/share/info" + ^^^^^^ "#{prefix}/share" should be "#{share}" + ^^^^^^^^^^^ "#{prefix}/share/info" should be "#{info}" + end end - EOS + RUBY + end - expected_offenses = [{ message: 'Use ruby-macho instead of calling "install_name_tool"', - severity: :convention, - line: 4, - column: 10, - source: source }] + it "formula path shortcut : man8" do + expect_offense(<<~'RUBY') + class Foo < Formula + desc "foo" + url 'http://example.com/foo-1.0.tgz' + def install + system "./configure", "--MANDIR=#{prefix}/share/man/man8" + ^^^^^^ "#{prefix}/share" should be "#{share}" + ^^^^^^^^^^^^^^^ "#{prefix}/share/man/man8" should be "#{man8}" + end + end + RUBY + end - inspect_source(source) + it "dependecies which have to vendored" do + expect_offense(<<~RUBY) + class Foo < Formula + desc "foo" + url 'http://example.com/foo-1.0.tgz' + depends_on "lpeg" => :lua51 + ^^^^^ lua modules should be vendored rather than use deprecated depends_on \"lpeg\" => :lua51` + end + RUBY + end - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + it "manually setting env" do + expect_offense(<<~RUBY) + class Foo < Formula + desc "foo" + url 'http://example.com/foo-1.0.tgz' + system "export", "var=value" + ^^^^^^ Use ENV instead of invoking 'export' to modify the environment + end + RUBY end - it "with ruby-macho alternatives audit exempted formula" do - source = <<~EOS - class Cctools < Formula + it "dependencies with invalid options" do + expect_offense(<<~RUBY) + class Foo < Formula desc "foo" url 'http://example.com/foo-1.0.tgz' - system "install_name_tool", "-id" + depends_on "foo" => "with-bar" + ^^^^^^^^^^^^^^^^^^^ Dependency foo should not use option with-bar end - EOS + RUBY + end - inspect_source(source, "/homebrew-core/Formula/cctools.rb") - expect(cop.offenses).to eq([]) + it "inspecting version manually" do + expect_offense(<<~RUBY) + class Foo < Formula + desc "foo" + url 'http://example.com/foo-1.0.tgz' + if version == "HEAD" + ^^^^^^^^^^^^^^^^^ Use 'build.head?' instead of inspecting 'version' + foo() + end + end + RUBY end - it "with npm install without language::Node args" do - source = <<~EOS + it "deprecated ENV.fortran usage" do + expect_offense(<<~RUBY) class Foo < Formula desc "foo" url 'http://example.com/foo-1.0.tgz' - system "npm", "install" + test do + ENV.fortran + ^^^^^^^^^^^ Use `depends_on :fortran` instead of `ENV.fortran` + end end - EOS + RUBY + end - expected_offenses = [{ message: "Use Language::Node for npm install args", - severity: :convention, - line: 4, - column: 2, - source: source }] + it "deprecated ARGV.include? (--HEAD) usage" do + expect_offense(<<~RUBY) + class Foo < Formula + desc "foo" + url 'http://example.com/foo-1.0.tgz' + test do + head = ARGV.include? "--HEAD" + ^^^^^^ Use "if build.head?" instead + end + end + RUBY + end - inspect_source(source) + it "deprecated MACOS_VERSION const usage" do + expect_offense(<<~RUBY) + class Foo < Formula + desc "foo" + url 'http://example.com/foo-1.0.tgz' + test do + version = MACOS_VERSION + ^^^^^^^^^^^^^ Use MacOS.version instead of MACOS_VERSION + end + end + RUBY + end - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + it "deprecated if build.with? conditional dependency" do + expect_offense(<<~RUBY) + class Foo < Formula + desc "foo" + url 'http://example.com/foo-1.0.tgz' + depends_on "foo" if build.with? "with-foo" + ^^^^^^^^^^^^^^^^ Replace depends_on "foo" if build.with? "with-foo" with depends_on "foo" => :optional + end + RUBY end - it "with npm install without language::Node args in kibana" do - source = <<~EOS - class KibanaAT44 < Formula + it "unless conditional dependency with build.without?" do + expect_offense(<<~RUBY) + class Foo < Formula desc "foo" url 'http://example.com/foo-1.0.tgz' - system "npm", "install" + depends_on :foo unless build.without? "foo" + ^^^^^^^^^^^^^^^ Replace depends_on :foo unless build.without? "foo" with depends_on :foo => :recommended end - EOS + RUBY + end - inspect_source(source, "/homebrew-core/Formula/kibana@4.4.rb") - expect(cop.offenses).to eq([]) + it "unless conditional dependency with build.include?" do + expect_offense(<<~RUBY) + class Foo < Formula + desc "foo" + url 'http://example.com/foo-1.0.tgz' + depends_on :foo unless build.include? "without-foo" + ^^^^^^^^^^^^^^^ Replace depends_on :foo unless build.include? "without-foo" with depends_on :foo => :recommended + end + RUBY end end end diff --git a/Library/Homebrew/test/rubocops/options_cop_spec.rb b/Library/Homebrew/test/rubocops/options_cop_spec.rb index 1ed6ee740..b888f4315 100644 --- a/Library/Homebrew/test/rubocops/options_cop_spec.rb +++ b/Library/Homebrew/test/rubocops/options_cop_spec.rb @@ -1,32 +1,16 @@ -require "rubocop" -require "rubocop/rspec/support" -require_relative "../../extend/string" require_relative "../../rubocops/options_cop" describe RuboCop::Cop::FormulaAudit::Options do subject(:cop) { described_class.new } - context "When auditing options" do - it "32-bit" do - source = <<~EOS - class Foo < Formula - url 'http://example.com/foo-1.0.tgz' - option "32-bit", "with 32-bit" - end - EOS - - expected_offenses = [{ message: described_class::DEPRECATION_MSG, - severity: :convention, - line: 3, - column: 10, - source: source }] - - inspect_source(source) - - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) + it "reports an offense when using the 32-bit option" do + expect_offense(<<~RUBY) + class Foo < Formula + url 'http://example.com/foo-1.0.tgz' + option "32-bit", "with 32-bit" + ^^^^^^ macOS has been 64-bit only since 10.6 so 32-bit options are deprecated. end - end + RUBY end end @@ -35,71 +19,34 @@ describe RuboCop::Cop::FormulaAuditStrict::Options do context "When auditing options strictly" do it "with universal" do - source = <<~EOS + expect_offense(<<~RUBY) class Foo < Formula url 'http://example.com/foo-1.0.tgz' option :universal + ^^^^^^^^^^^^^^^^^ macOS has been 64-bit only since 10.6 so universal options are deprecated. end - EOS - - expected_offenses = [{ message: described_class::DEPRECATION_MSG, - severity: :convention, - line: 3, - column: 2, - source: source }] - - inspect_source(source) - - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + RUBY end it "with deprecated options" do - source = <<~EOS + expect_offense(<<~RUBY) class Foo < Formula url 'http://example.com/foo-1.0.tgz' option :cxx11 option "examples", "with-examples" + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Options should begin with with/without. Migrate '--examples' with `deprecated_option`. end - EOS - - MSG_1 = "Options should begin with with/without."\ - " Migrate '--examples' with `deprecated_option`.".freeze - expected_offenses = [{ message: MSG_1, - severity: :convention, - line: 4, - column: 2, - source: source }] - - inspect_source(source) - - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + RUBY end it "with misc deprecated options" do - source = <<~EOS + expect_offense(<<~RUBY) class Foo < Formula url 'http://example.com/foo-1.0.tgz' option "without-check" + ^^^^^^^^^^^^^^^^^^^^^^ Use '--without-test' instead of '--without-check'. Migrate '--without-check' with `deprecated_option`. end - EOS - - MSG_2 = "Use '--without-test' instead of '--without-check'."\ - " Migrate '--without-check' with `deprecated_option`.".freeze - expected_offenses = [{ message: MSG_2, - severity: :convention, - line: 3, - column: 2, - source: source }] - - inspect_source(source) - - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + RUBY end end end @@ -109,24 +56,13 @@ describe RuboCop::Cop::NewFormulaAudit::Options do context "When auditing options for a new formula" do it "with deprecated options" do - source = <<~EOS + expect_offense(<<~RUBY) class Foo < Formula url 'http://example.com/foo-1.0.tgz' deprecated_option "examples" => "with-examples" + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ New Formula should not use `deprecated_option` end - EOS - - expected_offenses = [{ message: described_class::MSG, - severity: :convention, - line: 3, - column: 2, - source: source }] - - inspect_source(source) - - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + RUBY end end end diff --git a/Library/Homebrew/test/rubocops/patches_cop_spec.rb b/Library/Homebrew/test/rubocops/patches_cop_spec.rb index fdecb676e..771171160 100644 --- a/Library/Homebrew/test/rubocops/patches_cop_spec.rb +++ b/Library/Homebrew/test/rubocops/patches_cop_spec.rb @@ -1,6 +1,3 @@ -require "rubocop" -require "rubocop/rspec/support" -require_relative "../../extend/string" require_relative "../../rubocops/patches_cop" describe RuboCop::Cop::FormulaAudit::Patches do @@ -8,37 +5,24 @@ describe RuboCop::Cop::FormulaAudit::Patches do context "When auditing legacy patches" do it "When there is no legacy patch" do - source = <<~EOS + expect_no_offenses(<<~RUBY) class Foo < Formula url 'http://example.com/foo-1.0.tgz' end - EOS - inspect_source(source) - expect(cop.offenses).to eq([]) + RUBY end it "Formula with `def patches`" do - source = <<~EOS + expect_offense(<<~RUBY) class Foo < Formula homepage "ftp://example.com/foo" url "http://example.com/foo-1.0.tgz" def patches + ^^^^^^^^^^^ Use the patch DSL instead of defining a 'patches' method DATA end end - EOS - - expected_offenses = [{ message: "Use the patch DSL instead of defining a 'patches' method", - severity: :convention, - line: 4, - column: 2, - source: source }] - - inspect_source(source) - - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + RUBY end it "Patch URLs" do @@ -120,7 +104,10 @@ describe RuboCop::Cop::FormulaAudit::Patches do source: source }] end expected_offense.zip([cop.offenses.last]).each do |expected, actual| - expect_offense(expected, actual) + expect(actual.message).to eq(expected[:message]) + expect(actual.severity).to eq(expected[:severity]) + expect(actual.line).to eq(expected[:line]) + expect(actual.column).to eq(expected[:column]) end end end @@ -157,7 +144,10 @@ describe RuboCop::Cop::FormulaAudit::Patches do inspect_source(source) expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) + expect(actual.message).to eq(expected[:message]) + expect(actual.severity).to eq(expected[:severity]) + expect(actual.line).to eq(expected[:line]) + expect(actual.column).to eq(expected[:column]) end end end @@ -233,7 +223,10 @@ describe RuboCop::Cop::FormulaAudit::Patches do source: source }] end expected_offense.zip([cop.offenses.last]).each do |expected, actual| - expect_offense(expected, actual) + expect(actual.message).to eq(expected[:message]) + expect(actual.severity).to eq(expected[:severity]) + expect(actual.line).to eq(expected[:line]) + expect(actual.column).to eq(expected[:column]) end end end diff --git a/Library/Homebrew/test/rubocops/text_cop_spec.rb b/Library/Homebrew/test/rubocops/text_cop_spec.rb index dbddff1ad..84e2344c5 100644 --- a/Library/Homebrew/test/rubocops/text_cop_spec.rb +++ b/Library/Homebrew/test/rubocops/text_cop_spec.rb @@ -1,6 +1,3 @@ -require "rubocop" -require "rubocop/rspec/support" -require_relative "../../extend/string" require_relative "../../rubocops/text_cop" describe RuboCop::Cop::FormulaAudit::Text do @@ -8,189 +5,114 @@ describe RuboCop::Cop::FormulaAudit::Text do context "When auditing formula text" do it "with both openssl and libressl optional dependencies" do - source = <<~EOS + expect_offense(<<~RUBY) class Foo < Formula url "http://example.com/foo-1.0.tgz" homepage "http://example.com" depends_on "openssl" depends_on "libressl" => :optional + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Formulae should not depend on both OpenSSL and LibreSSL (even optionally). end - EOS - - expected_offenses = [{ message: "Formulae should not depend on both OpenSSL and LibreSSL (even optionally).", - severity: :convention, - line: 6, - column: 2, - source: source }] - - inspect_source(source) - - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + RUBY end it "with both openssl and libressl dependencies" do - source = <<~EOS + expect_offense(<<~RUBY) class Foo < Formula url "http://example.com/foo-1.0.tgz" homepage "http://example.com" depends_on "openssl" depends_on "libressl" + ^^^^^^^^^^^^^^^^^^^^^ Formulae should not depend on both OpenSSL and LibreSSL (even optionally). end - EOS - - expected_offenses = [{ message: "Formulae should not depend on both OpenSSL and LibreSSL (even optionally).", - severity: :convention, - line: 6, - column: 2, - source: source }] - - inspect_source(source) - - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + RUBY end it "When xcodebuild is called without SYMROOT" do - source = <<~EOS + expect_offense(<<~RUBY) class Foo < Formula url "http://example.com/foo-1.0.tgz" homepage "http://example.com" def install xcodebuild "-project", "meow.xcodeproject" + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ xcodebuild should be passed an explicit \"SYMROOT\" end end - EOS - - expected_offenses = [{ message: "xcodebuild should be passed an explicit \"SYMROOT\"", - severity: :convention, - line: 6, - column: 4, - source: source }] - - inspect_source(source) - - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + RUBY end it "When xcodebuild is called without any args" do - source = <<~EOS + expect_offense(<<~RUBY) class Foo < Formula url "http://example.com/foo-1.0.tgz" homepage "http://example.com" def install xcodebuild + ^^^^^^^^^^ xcodebuild should be passed an explicit \"SYMROOT\" end end - EOS - - expected_offenses = [{ message: "xcodebuild should be passed an explicit \"SYMROOT\"", - severity: :convention, - line: 6, - column: 4, - source: source }] - - inspect_source(source) - - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + RUBY end it "When go get is executed" do - source = <<~EOS + expect_offense(<<~RUBY) class Foo < Formula url "http://example.com/foo-1.0.tgz" homepage "http://example.com" def install system "go", "get", "bar" + ^^^^^^^^^^^^^^^^^^^^^^^^^ Formulae should not use `go get`. If non-vendored resources are required use `go_resource`s. end end - EOS - - expected_offenses = [{ message: "Formulae should not use `go get`. If non-vendored resources are required use `go_resource`s.", - severity: :convention, - line: 6, - column: 4, - source: source }] - - inspect_source(source) - - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + RUBY end it "When xcodebuild is executed" do - source = <<~EOS + expect_offense(<<~RUBY) class Foo < Formula url "http://example.com/foo-1.0.tgz" homepage "http://example.com" def install system "xcodebuild", "foo", "bar" + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ use \"xcodebuild *args\" instead of \"system 'xcodebuild', *args\" end end - EOS - - expected_offenses = [{ message: "use \"xcodebuild *args\" instead of \"system 'xcodebuild', *args\"", - severity: :convention, - line: 6, - column: 4, - source: source }] - - inspect_source(source) - - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + RUBY end it "When scons is executed" do - source = <<~EOS + expect_offense(<<~RUBY) class Foo < Formula url "http://example.com/foo-1.0.tgz" homepage "http://example.com" def install system "scons", "foo", "bar" + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ use \"scons *args\" instead of \"system 'scons', *args\" end end - EOS - - expected_offenses = [{ message: "use \"scons *args\" instead of \"system 'scons', *args\"", - severity: :convention, - line: 6, - column: 4, - source: source }] - - inspect_source(source) - - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + RUBY end it "When plist_options are not defined when using a formula-defined plist", :ruby23 do - source = <<~RUBY + expect_offense(<<~RUBY) class Foo < Formula url "http://example.com/foo-1.0.tgz" homepage "http://example.com" def install system "xcodebuild", "foo", "bar" + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ use \"xcodebuild *args\" instead of \"system 'xcodebuild', *args\" end def plist + ^^^^^^^^^ Please set plist_options when using a formula-defined plist. <<~XML <?xml version="1.0" encoding="UTF-8"?> <!DOCTYPE plist PUBLIC "-//Apple Computer//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd"> @@ -204,23 +126,12 @@ describe RuboCop::Cop::FormulaAudit::Text do end end RUBY - - expected_offenses = [{ message: "Please set plist_options when using a formula-defined plist.", - severity: :convention, - line: 9, - column: 2, - source: source }] - - inspect_source(source) - - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end end it "When language/go is require'd" do - source = <<~EOS + expect_offense(<<~RUBY) require "language/go" + ^^^^^^^^^^^^^^^^^^^^^ require "language/go" is unnecessary unless using `go_resource`s class Foo < Formula url "http://example.com/foo-1.0.tgz" @@ -228,30 +139,20 @@ describe RuboCop::Cop::FormulaAudit::Text do def install system "go", "get", "bar" + ^^^^^^^^^^^^^^^^^^^^^^^^^ Formulae should not use `go get`. If non-vendored resources are required use `go_resource`s. end end - EOS - - expected_offenses = [{ message: "require \"language/go\" is unnecessary unless using `go_resource`s", - severity: :convention, - line: 1, - column: 0, - source: source }] - - inspect_source(source) - - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + RUBY end it "When formula uses virtualenv and also `setuptools` resource" do - source = <<~EOS + expect_offense(<<~RUBY) class Foo < Formula url "http://example.com/foo-1.0.tgz" homepage "http://example.com" resource "setuptools" do + ^^^^^^^^^^^^^^^^^^^^^ Formulae using virtualenvs do not need a `setuptools` resource. url "https://foo.com/foo.tar.gz" sha256 "db0904a28253cfe53e7dedc765c71596f3c53bb8a866ae50123320ec1a7b73fd" end @@ -260,51 +161,21 @@ describe RuboCop::Cop::FormulaAudit::Text do virtualenv_create(libexec) end end - EOS - - expected_offenses = [{ message: "Formulae using virtualenvs do not need a `setuptools` resource.", - severity: :convention, - line: 5, - column: 2, - source: source }] - - inspect_source(source) - - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end + RUBY end it "When Formula.factory(name) is used" do - source = <<~EOS + expect_offense(<<~RUBY) class Foo < Formula url "http://example.com/foo-1.0.tgz" homepage "http://example.com" def install Formula.factory(name) + ^^^^^^^^^^^^^^^^^^^^^ \"Formula.factory(name)\" is deprecated in favor of \"Formula[name]\" end end - EOS - - expected_offenses = [{ message: "\"Formula.factory(name)\" is deprecated in favor of \"Formula[name]\"", - severity: :convention, - line: 6, - column: 4, - source: source }] - - inspect_source(source) - - expected_offenses.zip(cop.offenses).each do |expected, actual| - expect_offense(expected, actual) - end - end - - def expect_offense(expected, actual) - expect(actual.message).to eq(expected[:message]) - expect(actual.severity).to eq(expected[:severity]) - expect(actual.line).to eq(expected[:line]) - expect(actual.column).to eq(expected[:column]) + RUBY end end end diff --git a/Library/Homebrew/test/rubocops/urls_cop_spec.rb b/Library/Homebrew/test/rubocops/urls_cop_spec.rb index 0bda7f110..494028bd5 100644 --- a/Library/Homebrew/test/rubocops/urls_cop_spec.rb +++ b/Library/Homebrew/test/rubocops/urls_cop_spec.rb @@ -1,6 +1,3 @@ -require "rubocop" -require "rubocop/rspec/support" -require_relative "../../extend/string" require_relative "../../rubocops/urls_cop" describe RuboCop::Cop::FormulaAudit::Urls do @@ -129,65 +126,40 @@ describe RuboCop::Cop::FormulaAudit::Urls do inspect_source(source) expected_offenses.zip(cop.offenses.reverse).each do |expected, actual| - expect_offense(expected, actual) + expect(actual.message).to eq(expected[:message]) + expect(actual.severity).to eq(expected[:severity]) + expect(actual.line).to eq(expected[:line]) + expect(actual.column).to eq(expected[:column]) end end end it "with offenses in stable/devel/head block" do - formulas = [{ - "url" => "git://github.com/foo.git", - "msg" => "Please use https:// for git://github.com/foo.git", - "col" => 4, - }] - formulas.each do |formula| - source = <<~EOS - class Foo < Formula - desc "foo" - url "https://foo.com" + expect_offense(<<~RUBY) + class Foo < Formula + desc "foo" + url "https://foo.com" - devel do - url "#{formula["url"]}", - :tag => "v1.0.0-alpha.1", - :revision => "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" - version "1.0.0-alpha.1" - end + devel do + url "git://github.com/foo.git", + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Please use https:// for git://github.com/foo.git + :tag => "v1.0.0-alpha.1", + :revision => "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + version "1.0.0-alpha.1" end - EOS - expected_offenses = [{ message: formula["msg"], - severity: :convention, - line: 6, - column: formula["col"], - source: source }] - - inspect_source(source) - - expected_offenses.zip(cop.offenses.reverse).each do |expected, actual| - expect_offense(expected, actual) end - end + RUBY end it "with duplicate mirror" do - source = <<~EOS + expect_offense(<<~RUBY) class Foo < Formula desc "foo" url "https://ftpmirror.fnu.org/foo/foo-1.0.tar.gz" mirror "https://ftpmirror.fnu.org/foo/foo-1.0.tar.gz" + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ URL should not be duplicated as a mirror: https://ftpmirror.fnu.org/foo/foo-1.0.tar.gz end - EOS - - expected_offenses = [{ message: "URL should not be duplicated as a mirror: https://ftpmirror.fnu.org/foo/foo-1.0.tar.gz", - severity: :convention, - line: 4, - column: 2, - source: source }] - - inspect_source(source) - - expected_offenses.zip(cop.offenses.reverse).each do |expected, actual| - expect_offense(expected, actual) - end + RUBY end end end @@ -195,42 +167,31 @@ end describe RuboCop::Cop::FormulaAuditStrict::PyPiUrls do subject(:cop) { described_class.new } - context "When auditing urls" do - it "with pypi offenses" do - formulas = [{ - "url" => "https://pypi.python.org/packages/source/foo/foo-0.1.tar.gz", - "msg" => "https://pypi.python.org/packages/source/foo/foo-0.1.tar.gz should be `https://files.pythonhosted.org/packages/source/foo/foo-0.1.tar.gz`", - "col" => 2, - "corrected_url" =>"https://files.pythonhosted.org/packages/source/foo/foo-0.1.tar.gz", - }] - formulas.each do |formula| - source = <<~EOS - class Foo < Formula - desc "foo" - url "#{formula["url"]}" - end - EOS - corrected_source = <<~EOS - class Foo < Formula - desc "foo" - url "#{formula["corrected_url"]}" - end - EOS - expected_offenses = [{ message: formula["msg"], - severity: :convention, - line: 3, - column: formula["col"], - source: source }] + context "when a pypi.python.org URL is used" do + it "reports an offense" do + expect_offense(<<~RUBY) + class Foo < Formula + desc "foo" + url "https://pypi.python.org/packages/source/foo/foo-0.1.tar.gz" + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ https://pypi.python.org/packages/source/foo/foo-0.1.tar.gz should be `https://files.pythonhosted.org/packages/source/foo/foo-0.1.tar.gz` + end + RUBY + end - inspect_source(source) - # Check for expected offenses - expected_offenses.zip(cop.offenses.reverse).each do |expected, actual| - expect_offense(expected, actual) + it "support auto-correction" do + corrected = autocorrect_source(<<~RUBY) + class Foo < Formula + desc "foo" + url "https://pypi.python.org/packages/source/foo/foo-0.1.tar.gz" end - # Check for expected auto corrected source - new_source = autocorrect_source(source) - expect(new_source).to eq(corrected_source) - end + RUBY + + expect(corrected).to eq <<~RUBY + class Foo < Formula + desc "foo" + url "https://files.pythonhosted.org/packages/source/foo/foo-0.1.tar.gz" + end + RUBY end end end diff --git a/Library/Homebrew/test/spec_helper.rb b/Library/Homebrew/test/spec_helper.rb index a5821ee14..c40a72fcc 100644 --- a/Library/Homebrew/test/spec_helper.rb +++ b/Library/Homebrew/test/spec_helper.rb @@ -2,6 +2,8 @@ require "find" require "pathname" require "rspec/its" require "rspec/wait" +require "rubocop" +require "rubocop/rspec/support" require "set" if ENV["HOMEBREW_TESTS_COVERAGE"] @@ -23,7 +25,6 @@ require "test/support/helper/fixtures" require "test/support/helper/formula" require "test/support/helper/mktmpdir" require "test/support/helper/output_as_tty" -require "test/support/helper/rubocop" require "test/support/helper/spec/shared_context/homebrew_cask" if OS.mac? require "test/support/helper/spec/shared_context/integration_test" @@ -43,11 +44,14 @@ RSpec.configure do |config| config.filter_run_when_matching :focus + config.include(FileUtils) + + config.include(RuboCop::RSpec::ExpectOffense) + config.include(Test::Helper::Fixtures) config.include(Test::Helper::Formula) config.include(Test::Helper::MkTmpDir) config.include(Test::Helper::OutputAsTTY) - config.include(Test::Helper::RuboCop) config.before(:each, :needs_compat) do skip "Requires compatibility layer." if ENV["HOMEBREW_NO_COMPAT"] diff --git a/Library/Homebrew/test/support/helper/rubocop.rb b/Library/Homebrew/test/support/helper/rubocop.rb deleted file mode 100644 index 351f2ad82..000000000 --- a/Library/Homebrew/test/support/helper/rubocop.rb +++ /dev/null @@ -1,13 +0,0 @@ -module Test - module Helper - module RuboCop - def expect_offense(expected, actual) - expect(actual).to_not be_nil - expect(actual.message).to eq(expected[:message]) - expect(actual.severity).to eq(expected[:severity]) - expect(actual.line).to eq(expected[:line]) - expect(actual.column).to eq(expected[:column]) - end - end - end -end diff --git a/Library/Homebrew/test/tap_spec.rb b/Library/Homebrew/test/tap_spec.rb index 76af27b92..cb908da9f 100644 --- a/Library/Homebrew/test/tap_spec.rb +++ b/Library/Homebrew/test/tap_spec.rb @@ -1,6 +1,4 @@ describe Tap do - include FileUtils - alias_matcher :have_formula_file, :be_formula_file alias_matcher :have_custom_remote, :be_custom_remote @@ -307,8 +305,6 @@ describe Tap do end describe CoreTap do - include FileUtils - specify "attributes" do expect(subject.user).to eq("Homebrew") expect(subject.repo).to eq("core") diff --git a/Library/Homebrew/utils/github.rb b/Library/Homebrew/utils/github.rb index f88d52403..e73951c8d 100644 --- a/Library/Homebrew/utils/github.rb +++ b/Library/Homebrew/utils/github.rb @@ -235,7 +235,7 @@ module GitHub def issues_for_formula(name, options = {}) tap = options[:tap] || CoreTap.instance - search_issues(name, state: "open", repo: "#{tap.user}/homebrew-#{tap.repo}") + search_issues(name, state: "open", repo: "#{tap.user}/homebrew-#{tap.repo}", in: "title") end def print_pull_requests_matching(query) |