Merge pull request #9279 from AriaXLi/PUP-11993/more_style_cops

(PUP-11993) More Style cops!

.rubocop.yml

@@ -33,6 +33,24 @@ Layout/IndentationWidth:
 Layout/LineEndStringConcatenationIndentation:
   Enabled: true
 
+# Enabling this cop will remove raising RuntimeErrors and cause spec test
+# failures
+Style/RedundantException:
+  Exclude:
+    - 'lib/puppet/file_bucket/dipper.rb'
+    - 'lib/puppet/forge.rb'
+    - 'lib/puppet/module_tool/applications/unpacker.rb'
+    - 'lib/puppet/module_tool/local_tarball.rb'
+    - 'lib/puppet/module_tool/tar.rb'
+    - 'lib/puppet/pops/types/class_loader.rb'
+
+# Enabling this cop causes a plethora of failed rspec tests, mostly
+# Errno::ENAMETOOLONG and Puppet::Context::UndefinedBindingError: Unable to
+# lookup 'environments' errors
+Style/RedundantSelfAssignment:
+  Exclude:
+    - 'lib/puppet/context.rb'
+
 # Explicitly enables this cop new in 1.7
 Layout/SpaceBeforeBrackets:
   Enabled: true

.rubocop_todo.yml

@@ -128,6 +128,12 @@ Lint/RedundantDirGlobSort: # new in 1.8
 Lint/RedundantRegexpQuantifiers: # new in 1.53
   Enabled: false
 
+# Unsure how the changes in portage.rb from Lint/RedundantSplatExpansion impact
+# the code
+Lint/RedundantSplatExpansion:
+  Exclude:
+    - 'lib/puppet/provider/package/portage.rb'
+
 Lint/RefinementImportMethods: # new in 1.27
   Enabled: false
 
@@ -646,76 +652,10 @@ Style/OptionalBooleanParameter:
 Style/PreferredHashMethods:
   Enabled: false
 
-# This cop supports safe auto-correction (--auto-correct).
-Style/RedundantConditional:
-  Exclude:
-    - 'lib/puppet/type/exec.rb'
-    - 'lib/puppet/type/file.rb'
-
-# This cop supports safe auto-correction (--auto-correct).
-Style/RedundantException:
-  Exclude:
-    - 'lib/puppet/file_bucket/dipper.rb'
-    - 'lib/puppet/forge.rb'
-    - 'lib/puppet/module_tool/applications/unpacker.rb'
-    - 'lib/puppet/module_tool/local_tarball.rb'
-    - 'lib/puppet/module_tool/tar.rb'
-    - 'lib/puppet/pops/types/class_loader.rb'
-
 # This cop supports unsafe auto-correction (--auto-correct-all).
-# Configuration parameters: SafeForConstants.
-Style/RedundantFetchBlock:
-  Exclude:
-    - 'lib/puppet/pops/types/p_sem_ver_range_type.rb'
-
-# This cop supports safe auto-correction (--auto-correct).
-Style/RedundantFileExtensionInRequire:
-  Exclude:
-    - 'lib/puppet/parser.rb'
-    - 'lib/puppet/type/file/content.rb'
-    - 'lib/puppet/util/rdoc/parser.rb'
-    - 'lib/puppet/util/rdoc/parser/puppet_parser_rdoc2.rb'
-
-# This cop supports safe auto-correction (--auto-correct).
 Style/RedundantInterpolation:
   Enabled: false
 
-# This cop supports safe auto-correction (--auto-correct).
-Style/RedundantParentheses:
-  Enabled: false
-
-# This cop supports safe auto-correction (--auto-correct).
-Style/RedundantPercentQ:
-  Exclude:
-    - 'lib/puppet/defaults.rb'
-    - 'lib/puppet/indirector/catalog/store_configs.rb'
-    - 'lib/puppet/indirector/facts/store_configs.rb'
-    - 'lib/puppet/indirector/node/store_configs.rb'
-    - 'lib/puppet/indirector/resource/store_configs.rb'
-    - 'lib/puppet/provider/package/dpkg.rb'
-    - 'lib/puppet/provider/package/rpm.rb'
-    - 'lib/puppet/type.rb'
-    - 'lib/puppet/util/at_fork/solaris.rb'
-
-# This cop supports safe auto-correction (--auto-correct).
-Style/RedundantRegexpCharacterClass:
-  Exclude:
-    - 'lib/puppet/application/apply.rb'
-    - 'lib/puppet/module.rb'
-    - 'lib/puppet/pops/lookup/interpolation.rb'
-    - 'lib/puppet/pops/parser/lexer2.rb'
-    - 'lib/puppet/pops/parser/slurp_support.rb'
-    - 'lib/puppet/pops/patterns.rb'
-    - 'lib/puppet/pops/pcore.rb'
-    - 'lib/puppet/provider/package/openbsd.rb'
-    - 'lib/puppet/settings/config_file.rb'
-    - 'lib/puppet/type/exec.rb'
-    - 'lib/puppet/util/command_line/trollop.rb'
-
-# This cop supports safe auto-correction (--auto-correct).
-Style/RedundantRegexpEscape:
-  Enabled: false
-
 # This cop supports safe auto-correction (--auto-correct).
 # Configuration parameters: AllowMultipleReturnValues.
 Style/RedundantReturn:
@@ -725,44 +665,6 @@ Style/RedundantReturn:
 Style/RedundantSelf:
   Enabled: false
 
-# This cop supports unsafe auto-correction (--auto-correct-all).
-Style/RedundantSelfAssignment:
-  Exclude:
-    - 'lib/puppet/context.rb'
-
-# This cop supports unsafe auto-correction (--auto-correct-all).
-Style/RedundantSort:
-  Exclude:
-    - 'lib/puppet/interface/face_collection.rb'
-    - 'lib/puppet/module/plan.rb'
-    - 'lib/puppet/module_tool/applications/unpacker.rb'
-    - 'lib/puppet/provider/package/apt.rb'
-    - 'lib/puppet/provider/package/yum.rb'
-    - 'lib/puppet/provider/package/zypper.rb'
-
-# This cop supports safe auto-correction (--auto-correct).
-# Configuration parameters: EnforcedStyle, AllowInnerSlashes.
-# SupportedStyles: slashes, percent_r, mixed
-Style/RegexpLiteral:
-  Enabled: false
-
-# This cop supports safe auto-correction (--auto-correct).
-Style/RescueModifier:
-  Exclude:
-    - 'lib/puppet/file_serving/metadata.rb'
-    - 'lib/puppet/file_system/uniquefile.rb'
-    - 'lib/puppet/file_system/windows.rb'
-    - 'lib/puppet/module_tool/applications/application.rb'
-    - 'lib/puppet/module_tool/applications/installer.rb'
-    - 'lib/puppet/module_tool/applications/upgrader.rb'
-    - 'lib/puppet/module_tool/metadata.rb'
-    - 'lib/puppet/module_tool/shared_behaviors.rb'
-    - 'lib/puppet/provider/file/posix.rb'
-    - 'lib/puppet/util.rb'
-    - 'lib/puppet/util/execution.rb'
-    - 'lib/puppet/util/reference.rb'
-    - 'lib/puppet/util/windows/adsi.rb'
-
 # This cop supports safe auto-correction (--auto-correct).
 # Configuration parameters: EnforcedStyle.
 # SupportedStyles: implicit, explicit

lib/puppet/application/agent.rb

@@ -509,7 +509,7 @@ def enable_disable_client(agent)
   end
 
   def setup_agent
-    agent = Puppet::Agent.new(Puppet::Configurer, (!(Puppet[:onetime])))
+    agent = Puppet::Agent.new(Puppet::Configurer, !(Puppet[:onetime]))
 
     enable_disable_client(agent) if options[:enable] or options[:disable]
 

lib/puppet/application/apply.rb

@@ -224,7 +224,7 @@ def main
             $stderr.puts _("%{file} is not readable") % { file: file }
             exit(63)
           end
-          node.classes = Puppet::FileSystem.read(file, :encoding => 'utf-8').split(/[\s]+/)
+          node.classes = Puppet::FileSystem.read(file, :encoding => 'utf-8').split(/\s+/)
         end
       end
 

lib/puppet/application/describe.rb

@@ -11,7 +11,7 @@ def wrap(txt, opts)
     return "" unless txt && !txt.empty?
 
     work = (opts[:scrub] ? scrub(txt) : txt)
-    indent = (opts[:indent] || 0)
+    indent = opts[:indent] || 0
     textLen = @width - indent
     patt = Regexp.new("\\A(.{0,#{textLen}})[ \n]")
     prefix = " " * indent

lib/puppet/application/face_base.rb

@@ -205,7 +205,7 @@ def setup
     @arguments << options
 
     # If we don't have a rendering format, set one early.
-    self.render_as ||= (@action.render_as || :console)
+    self.render_as ||= @action.render_as || :console
   end
 
   def main

lib/puppet/defaults.rb

@@ -77,7 +77,7 @@ def self.default_vendormoduledir
   #   see the docs for Settings.define_settings
   ############################################################################################
 
-  AS_DURATION = %q{This setting can be a time interval in seconds (30 or 30s), minutes (30m), hours (6h), days (2d), or years (5y).}
+  AS_DURATION = 'This setting can be a time interval in seconds (30 or 30s), minutes (30m), hours (6h), days (2d), or years (5y).'
 
   # @api public
   # @param args [Puppet::Settings] the settings object to define default settings for
@@ -489,7 +489,7 @@ def self.initialize_default_settings!(settings)
     },
     :daemonize => {
         :type     => :boolean,
-        :default  => (!Puppet::Util::Platform.windows?),
+        :default  => !Puppet::Util::Platform.windows?,
         :desc     => "Whether to send the process into the background.  This defaults
           to true on POSIX systems, and to false on Windows (where Puppet
           currently cannot daemonize).",
@@ -650,7 +650,7 @@ def self.initialize_default_settings!(settings)
     :http_proxy_password =>{
       :default    => "none",
       :hook       => proc do |value|
-        if value =~ /[@!# \/]/
+        if value =~ %r{[@!# /]}
           raise "Passwords set in the http_proxy_password setting must be valid as part of a URL, and any reserved characters must be URL-encoded. We received: #{value}"
         end
       end,

lib/puppet/face/help.rb

@@ -147,7 +147,7 @@ def erb(name)
   # Return a list of applications that are not simply just stubs for Faces.
   def legacy_applications
     Puppet::Application.available_application_names.reject do |appname|
-      (is_face_app?(appname)) or (exclude_from_docs?(appname))
+      is_face_app?(appname) or exclude_from_docs?(appname)
     end.sort
   end
 
@@ -208,7 +208,7 @@ def horribly_extract_summary_from(appname)
     # formatted.  If we can't match the pattern we expect we return the empty
     # string to ensure we don't blow up in the summary. --daniel 2011-04-11
     while line = help.shift do # rubocop:disable Lint/AssignmentInCondition
-      md = /^puppet-#{appname}\([^\)]+\) -- (.*)$/.match(line)
+      md = /^puppet-#{appname}\([^)]+\) -- (.*)$/.match(line)
       if md
         return md[1]
       end

lib/puppet/file_serving/configuration.rb

@@ -56,7 +56,7 @@ def split_path(request)
 
     mount_name, path = request.key.split(File::Separator, 2)
 
-    raise(ArgumentError, _("Cannot find file: Invalid mount '%{mount_name}'") % { mount_name: mount_name }) unless mount_name =~ %r{^[-\w]+$}
+    raise(ArgumentError, _("Cannot find file: Invalid mount '%{mount_name}'") % { mount_name: mount_name }) unless mount_name =~ /^[-\w]+$/
     raise(ArgumentError, _("Cannot find file: Invalid relative path '%{path}'") % { path: path }) if path and path.split('/').include?('..')
 
     mount = find_mount(mount_name, request.environment)
@@ -71,7 +71,7 @@ def split_path(request)
       path = nil
     elsif path
       # Remove any double slashes that might have occurred
-      path = path.gsub(/\/+/, "/")
+      path = path.gsub(%r{/+}, "/")
     end
 
     return mount, path

lib/puppet/file_serving/fileset.rb

@@ -29,7 +29,7 @@ def self.merge(*filesets)
   def initialize(path, options = {})
     if Puppet::Util::Platform.windows?
       # REMIND: UNC path
-      path = path.chomp(File::SEPARATOR) unless path =~ /^[A-Za-z]:\/$/
+      path = path.chomp(File::SEPARATOR) unless path =~ %r{^[A-Za-z]:/$}
     else
       path = path.chomp(File::SEPARATOR) unless path == File::SEPARATOR
     end

lib/puppet/file_serving/metadata.rb

@@ -47,7 +47,7 @@ class MetaStat
 
     def initialize(stat, source_permissions)
       @stat = stat
-      @source_permissions_ignore = (!source_permissions || source_permissions == :ignore)
+      @source_permissions_ignore = !source_permissions || source_permissions == :ignore
     end
 
     def owner
@@ -111,16 +111,20 @@ def collect(source_permissions = nil)
 
     case stat.ftype
     when "file"
-      @checksum = ("{#{@checksum_type}}") + send("#{@checksum_type}_file", real_path).to_s
+      @checksum = "{#{@checksum_type}}" + send("#{@checksum_type}_file", real_path).to_s
     when "directory" # Always just timestamp the directory.
       @checksum_type = "ctime"
-      @checksum = ("{#{@checksum_type}}") + send("#{@checksum_type}_file", path).to_s
+      @checksum = "{#{@checksum_type}}" + send("#{@checksum_type}_file", path).to_s
     when "link"
       @destination = Puppet::FileSystem.readlink(real_path)
-      @checksum = ("{#{@checksum_type}}") + send("#{@checksum_type}_file", real_path).to_s rescue nil
+      @checksum = begin
+        "{#{@checksum_type}}" + send("#{@checksum_type}_file", real_path).to_s
+      rescue
+        nil
+      end
     when "fifo", "socket"
       @checksum_type = "none"
-      @checksum = ("{#{@checksum_type}}") + send("#{@checksum_type}_file", real_path).to_s
+      @checksum = "{#{@checksum_type}}" + send("#{@checksum_type}_file", real_path).to_s
     else
       raise ArgumentError, _("Cannot manage files of type %{file_type}") % { file_type: stat.ftype }
     end

lib/puppet/file_serving/mount.rb

@@ -18,7 +18,7 @@ def find(path, options)
 
   # Create our object.  It must have a name.
   def initialize(name)
-    unless name =~ %r{^[-\w]+$}
+    unless name =~ /^[-\w]+$/
       raise ArgumentError, _("Invalid mount name format '%{name}'") % { name: name }
     end
 

lib/puppet/file_system/path_pattern.rb

@@ -8,7 +8,7 @@ class PathPattern
     class InvalidPattern < Puppet::Error; end
 
     DOTDOT = '..'
-    ABSOLUTE_UNIX = /^\//
+    ABSOLUTE_UNIX = %r{^/}
     ABSOLUTE_WINDOWS = /^[a-z]:/i
     CURRENT_DRIVE_RELATIVE_WINDOWS = /^\\/
 

lib/puppet/file_system/uniquefile.rb

@@ -151,10 +151,14 @@ def tmpdir
     tmp = '.'
     [ENV.fetch('TMPDIR', nil), ENV.fetch('TMP', nil), ENV.fetch('TEMP', nil), @@systmpdir, '/tmp'].each do |dir|
       stat = File.stat(dir) if dir
-      if stat && stat.directory? && stat.writable?
-        tmp = dir
-        break
-      end rescue nil
+      begin
+        if stat && stat.directory? && stat.writable?
+          tmp = dir
+          break
+        end
+      rescue
+        nil
+      end
     end
     File.expand_path(tmp)
   end

lib/puppet/file_system/windows.rb

@@ -78,11 +78,19 @@ def unlink(*file_names)
 
     file_names.each do |file_name|
       file_name = file_name.to_s # handle PathName
-      stat = Puppet::Util::Windows::File.stat(file_name) rescue nil
+      stat = begin
+        Puppet::Util::Windows::File.stat(file_name)
+      rescue
+        nil
+      end
 
       # sigh, Ruby + Windows :(
       if !stat
-        ::File.unlink(file_name) rescue Dir.rmdir(file_name)
+        begin
+          ::File.unlink(file_name)
+        rescue
+          Dir.rmdir(file_name)
+        end
       elsif stat.ftype == 'directory'
         if Puppet::Util::Windows::File.symlink?(file_name)
           Dir.rmdir(file_name)

lib/puppet/forge.rb

@@ -231,7 +231,7 @@ def unpack(file, destination)
     end
 
     def deprecated?
-      @data['module'] && (!@data['module']['deprecated_at'].nil?)
+      @data['module'] && !@data['module']['deprecated_at'].nil?
     end
   end
 

lib/puppet/functions/abs.rb

@@ -53,9 +53,9 @@ def on_string(x)
     # version of this function.
     #
     case x
-    when %r{^-?(?:\d+)(?:\.\d+){1}$}
+    when /^-?(?:\d+)(?:\.\d+){1}$/
       x.to_f.abs
-    when %r{^-?\d+$}
+    when /^-?\d+$/
       x.to_i.abs
     else
       raise(ArgumentError, 'abs(): Requires float or integer to work with - was given non decimal string')

lib/puppet/functions/max.rb

@@ -146,7 +146,7 @@ def on_string(*args)
     assert_arg_count(args)
 
     args.max do |a, b|
-      if a.to_s =~ %r{\A^-?\d+([._eE]\d+)?\z} && b.to_s =~ %r{\A-?\d+([._eE]\d+)?\z}
+      if a.to_s =~ /\A^-?\d+([._eE]\d+)?\z/ && b.to_s =~ /\A-?\d+([._eE]\d+)?\z/
         Puppet.warn_once('deprecations', 'max_function_numeric_coerce_string',
                          _("The max() function's auto conversion of String to Numeric is deprecated - change to convert input before calling, or use lambda"))
         a.to_f <=> b.to_f
@@ -232,7 +232,7 @@ def on_any(*args)
     args.max do |a, b|
       as = a.to_s
       bs = b.to_s
-      if as =~ %r{\A^-?\d+([._eE]\d+)?\z} && bs =~ %r{\A-?\d+([._eE]\d+)?\z}
+      if as =~ /\A^-?\d+([._eE]\d+)?\z/ && bs =~ /\A-?\d+([._eE]\d+)?\z/
         Puppet.warn_once('deprecations', 'max_function_numeric_coerce_string',
                          _("The max() function's auto conversion of String to Numeric is deprecated - change to convert input before calling, or use lambda"))
         a.to_f <=> b.to_f