Merge pull request #9231 from tvpartytonight/PUP-11767

(PUP-11767) Enable more style cops

.rubocop.yml

@@ -208,3 +208,27 @@ Style/CaseEquality:
 
 Style/CaseLikeIf:
   Enabled: true
+
+Style/ClassCheck:
+  Enabled: true
+
+Style/ClassEqualityComparison:
+  Enabled: true
+
+Style/ClassMethods:
+  Enabled: true
+
+Style/CollectionCompact:
+  Enabled: true
+
+Style/ColonMethodCall:
+  Enabled: true
+
+Style/CombinableLoops:
+  Enabled: true
+
+Style/ColonMethodDefinition:
+  Enabled: true
+
+Style/DefWithParentheses:
+  Enabled: true

.rubocop_todo.yml

@@ -341,7 +341,10 @@ Style/CommentedKeyword:
 Style/ConditionalAssignment:
   Enabled: false
 
-# This cop supports safe auto-correction (--auto-correct).
+# Enabling this would require reworking Puppet's use of DateTime's #rfc2822, #httptime, and _strptime
+Style/DateTime:
+  Enabled: false
+
 Style/DefWithParentheses:
   Enabled: false
 

lib/puppet/application.rb

@@ -372,7 +372,7 @@ def app_defaults
   # Initialize application defaults. It's usually not necessary to override this method.
   # @return [void]
   # @api public
-  def initialize_app_defaults()
+  def initialize_app_defaults
     Puppet.settings.initialize_app_defaults(app_defaults)
   end
 

lib/puppet/application/apply.rb

@@ -366,7 +366,7 @@ def apply_catalog(catalog)
 
   # Returns facts or nil
   #
-  def get_facts()
+  def get_facts
     facts = nil
     unless Puppet[:node_name_fact].empty?
       # Collect our facts.
@@ -381,7 +381,7 @@ def get_facts()
 
   # Returns the node or raises and error if node not found.
   #
-  def get_node()
+  def get_node
     node = Puppet::Node.indirection.find(Puppet[:node_name_value])
     raise _("Could not find node %{node}") % { node: Puppet[:node_name_value] } unless node
 
@@ -390,7 +390,7 @@ def get_node()
 
   # Returns either a manifest (filename) or nil if apply should use content of Puppet[:code]
   #
-  def get_manifest()
+  def get_manifest
     manifest = nil
     # Set our code or file to use.
     if options[:code] or command_line.args.length == 0

lib/puppet/daemon.rb

@@ -58,7 +58,7 @@ def daemonize
 
   # Close stdin/stdout/stderr so that we can finish our transition into 'daemon' mode.
   # @return nil
-  def self.close_streams()
+  def self.close_streams
     Puppet.debug("Closing streams for daemon mode")
     begin
       $stdin.reopen "/dev/null"
@@ -68,15 +68,15 @@ def self.close_streams()
       Puppet.debug("Finished closing streams for daemon mode")
     rescue => detail
       Puppet.err "Could not start #{Puppet.run_mode.name}: #{detail}"
-      Puppet::Util::replace_file("/tmp/daemonout", 0644) do |f|
+      Puppet::Util.replace_file("/tmp/daemonout", 0644) do |f|
         f.puts "Could not start #{Puppet.run_mode.name}: #{detail}"
       end
       exit(12)
     end
   end
 
   # Convenience signature for calling Puppet::Daemon.close_streams
-  def close_streams()
+  def close_streams
     Puppet::Daemon.close_streams
   end
 

lib/puppet/face/generate.rb

@@ -58,7 +58,7 @@
         raise ArgumentError, _("The output directory '%{outputdir}' exists and is not a directory") % { outputdir: outputdir }
       end
 
-      Puppet::FileSystem::mkpath(outputdir)
+      Puppet::FileSystem.mkpath(outputdir)
 
       generator.generate(inputs, outputdir, options[:force])
 

lib/puppet/face/help.rb

@@ -160,7 +160,7 @@ def legacy_applications
   # @return [Array] An Array of Arrays.  The outer array contains one entry per application; each
   #  element in the outer array is a pair whose first element is a String containing the application
   #  name, and whose second element is a String containing the summary for that application.
-  def all_application_summaries()
+  def all_application_summaries
     available_application_names_special_sort().inject([]) do |result, appname|
       next result if exclude_from_docs?(appname)
 
@@ -196,7 +196,7 @@ def all_application_summaries()
   COMMON = 'Common:'
   SPECIALIZED = 'Specialized:'
   BLANK = "\n"
-  def available_application_names_special_sort()
+  def available_application_names_special_sort
     full_list = Puppet::Application.available_application_names
     a_list = full_list & %w{apply agent config help lookup module resource}
     a_list = a_list.sort

lib/puppet/file_bucket/file.rb

@@ -125,7 +125,7 @@ def checksum_data(base_method)
     end
 
     def to_binary
-      Puppet::FileSystem::binread(@path)
+      Puppet::FileSystem.binread(@path)
     end
   end
 end

lib/puppet/file_system/windows.rb

@@ -115,7 +115,7 @@ def chmod(mode, path)
 
   def read_preserve_line_endings(path)
     contents = path.read(:mode => 'rb', :encoding => 'bom|utf-8')
-    contents = path.read(:mode => 'rb', :encoding => "bom|#{Encoding::default_external.name}") unless contents.valid_encoding?
+    contents = path.read(:mode => 'rb', :encoding => "bom|#{Encoding.default_external.name}") unless contents.valid_encoding?
     contents = path.read unless contents.valid_encoding?
 
     contents

lib/puppet/functions/break.rb

@@ -37,7 +37,7 @@
   dispatch :break_impl do
   end
 
-  def break_impl()
+  def break_impl
     # get file, line if available, else they are set to nil
     file, line = Puppet::Pops::PuppetStack.top_of_stack
 

lib/puppet/generate/models/type/property.rb

@@ -61,7 +61,7 @@ def self.get_puppet_type(property)
             enum    = strings.empty? ? nil : "Enum[#{strings.join(', ')}]"
             pattern = regexes.empty? ? nil : "Pattern[#{regexes.join(', ')}]"
             boolean = strings.include?('\'true\'') || strings.include?('\'false\'') ? 'Boolean' : nil
-            variant = [boolean, enum, pattern].reject { |t| t.nil? }
+            variant = [boolean, enum, pattern].compact
             return variant[0] if variant.size == 1
 
             "Variant[#{variant.join(', ')}]"

lib/puppet/generate/type.rb

@@ -49,7 +49,7 @@ def format=(format)
         # @return [Boolean] Returns true if the output is up-to-date or false if not.
         def up_to_date?(outputdir)
           f = effective_output_path(outputdir)
-          Puppet::FileSystem::exist?(f) && (Puppet::FileSystem::stat(@path) <=> Puppet::FileSystem::stat(f)) <= 0
+          Puppet::FileSystem.exist?(f) && (Puppet::FileSystem.stat(@path) <=> Puppet::FileSystem.stat(f)) <= 0
         end
 
         # Gets the filename of the output file.

lib/puppet/indirector/json.rb

@@ -50,7 +50,7 @@ def path(name, ext = '.json')
 
   private
 
-  def data_dir()
+  def data_dir
     Puppet.run_mode.server? ? Puppet[:server_datadir] : Puppet[:client_datadir]
   end
 

lib/puppet/module_tool.rb

@@ -48,7 +48,7 @@ def self.username_and_modname_from(full_module_name)
     # @return [Pathname, nil] the root path of the module directory or nil if
     #   we cannot find one
     def self.find_module_root(path)
-      path = Pathname.new(path) if path.class == String
+      path = Pathname.new(path) if path.instance_of?(String)
 
       path.expand_path.ascend do |p|
         return p if is_module_root?(p)
@@ -63,7 +63,7 @@ def self.find_module_root(path)
     # @param path [Pathname, String] path to analyse
     # @return [Boolean] true if the path is a module root, false otherwise
     def self.is_module_root?(path)
-      path = Pathname.new(path) if path.class == String
+      path = Pathname.new(path) if path.instance_of?(String)
 
       FileTest.file?(path + 'metadata.json')
     end

lib/puppet/node/environment.rb

@@ -552,7 +552,7 @@ def hash
   end
 
   # not private so it can be called in tests
-  def self.extralibs()
+  def self.extralibs
     if ENV['PUPPETLIB']
       split_path(ENV['PUPPETLIB'])
     else

lib/puppet/pal/compiler.rb

@@ -25,8 +25,8 @@ def initialize(internal_compiler)
     #
     def call_function(function_name, *args, &block)
       # TRANSLATORS: do not translate variable name strings in these assertions
-      Pal::assert_non_empty_string(function_name, 'function_name', false)
-      Pal::assert_type(Pal::T_ANY_ARRAY, args, 'args', false)
+      Pal.assert_non_empty_string(function_name, 'function_name', false)
+      Pal.assert_type(Pal::T_ANY_ARRAY, args, 'args', false)
       internal_evaluator.evaluator.external_call_function(function_name, args, topscope, &block)
     end
 

lib/puppet/parser/ast/hostclass.rb

@@ -23,7 +23,7 @@ def instantiate(modname)
     return all_types
   end
 
-  def code()
+  def code
     @context[:code]
   end
 end

lib/puppet/parser/ast/pops_bridge.rb

@@ -227,7 +227,7 @@ def instantiate_NodeDefinition(o, modname)
       end
     end
 
-    def code()
+    def code
       Expression.new(:value => @value)
     end
 

lib/puppet/parser/ast/resource.rb

@@ -61,6 +61,6 @@ def evaluate(scope)
           resource
         end
       end
-    end.flatten.reject { |resource| resource.nil? }
+    end.flatten.compact
   end
 end

lib/puppet/parser/compiler.rb

@@ -164,7 +164,7 @@ def validate_catalog(validation_stage)
   end
 
   # Constructs the overrides for the context
-  def context_overrides()
+  def context_overrides
     {
       :current_environment => environment,
       :global_scope => @topscope, # 4x placeholder for new global scope
@@ -240,7 +240,7 @@ def evaluate_classes(classes, scope, lazy_evaluate = true)
     class_parameters = nil
     # if we are a param class, save the classes hash
     # and transform classes to be the keys
-    if classes.class == Hash
+    if classes.instance_of?(Hash)
       class_parameters = classes
       classes = classes.keys
     end
@@ -543,7 +543,7 @@ def initvars
     @resources = []
 
     # Make sure any external node classes are in our class list
-    if @node.classes.class == Hash
+    if @node.classes.instance_of?(Hash)
       @catalog.add_class(*@node.classes.keys)
     else
       @catalog.add_class(*@node.classes)

lib/puppet/parser/functions/assert_type.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-Puppet::Parser::Functions::newfunction(
+Puppet::Parser::Functions.newfunction(
   :assert_type,
   :type => :rvalue,
   :arity => -3,

lib/puppet/parser/functions/binary_file.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-Puppet::Parser::Functions::newfunction(
+Puppet::Parser::Functions.newfunction(
   :binary_file,
   :type => :rvalue,
   :arity => 1,

lib/puppet/parser/functions/break.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-Puppet::Parser::Functions::newfunction(
+Puppet::Parser::Functions.newfunction(
   :break,
   :arity => 0,
   :doc => <<~DOC

lib/puppet/parser/functions/contain.rb

@@ -3,7 +3,7 @@
 # Called within a class definition, establishes a containment
 # relationship with another class
 
-Puppet::Parser::Functions::newfunction(
+Puppet::Parser::Functions.newfunction(
   :contain,
   :arity => -2,
   :doc => "Contain one or more classes inside the current class. If any of

lib/puppet/parser/functions/create_resources.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-Puppet::Parser::Functions::newfunction(:create_resources, :arity => -3, :doc => <<-'ENDHEREDOC') do |args|
+Puppet::Parser::Functions.newfunction(:create_resources, :arity => -3, :doc => <<-'ENDHEREDOC') do |args|
     Converts a hash into a set of resources and adds them to the catalog.
 
     **Note**: Use this function selectively. It's generally better to write resources in

lib/puppet/parser/functions/defined.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-Puppet::Parser::Functions::newfunction(
+Puppet::Parser::Functions.newfunction(
   :defined,
   :type => :rvalue,
   :arity => -2,

lib/puppet/parser/functions/dig.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-Puppet::Parser::Functions::newfunction(
+Puppet::Parser::Functions.newfunction(
   :dig,
   :type => :rvalue,
   :arity => -1,

lib/puppet/parser/functions/digest.rb

@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 
 require_relative '../../../puppet/util/checksums'
-Puppet::Parser::Functions::newfunction(:digest, :type => :rvalue, :arity => 1, :doc => "Returns a hash value from a provided string using the digest_algorithm setting from the Puppet config file.") do |args|
+Puppet::Parser::Functions.newfunction(:digest, :type => :rvalue, :arity => 1, :doc => "Returns a hash value from a provided string using the digest_algorithm setting from the Puppet config file.") do |args|
   algo = Puppet[:digest_algorithm]
   Puppet::Util::Checksums.method(algo.intern).call args[0]
 end

lib/puppet/parser/functions/each.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-Puppet::Parser::Functions::newfunction(
+Puppet::Parser::Functions.newfunction(
   :each,
   :type => :rvalue,
   :arity => -3,

lib/puppet/parser/functions/epp.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-Puppet::Parser::Functions::newfunction(:epp, :type => :rvalue, :arity => -2, :doc =>
+Puppet::Parser::Functions.newfunction(:epp, :type => :rvalue, :arity => -2, :doc =>
 "Evaluates an Embedded Puppet (EPP) template file and returns the rendered text
 result as a String.
 

lib/puppet/parser/functions/fail.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-Puppet::Parser::Functions::newfunction(
+Puppet::Parser::Functions.newfunction(
   :fail,
   :arity => -1,
   :doc => <<~DOC

lib/puppet/parser/functions/file.rb

@@ -2,7 +2,7 @@
 
 require_relative '../../../puppet/file_system'
 
-Puppet::Parser::Functions::newfunction(
+Puppet::Parser::Functions.newfunction(
   :file, :arity => -2, :type => :rvalue,
          :doc => "Loads a file from a module and returns its contents as a string.
 

lib/puppet/parser/functions/filter.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-Puppet::Parser::Functions::newfunction(
+Puppet::Parser::Functions.newfunction(
   :filter,
   :type => :rvalue,
   :arity => -3,

lib/puppet/parser/functions/find_file.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-Puppet::Parser::Functions::newfunction(
+Puppet::Parser::Functions.newfunction(
   :find_file,
   :type => :rvalue,
   :arity => -2,

lib/puppet/parser/functions/fqdn_rand.rb

@@ -3,7 +3,7 @@
 require 'digest/md5'
 require 'digest/sha2'
 
-Puppet::Parser::Functions::newfunction(:fqdn_rand, :arity => -2, :type => :rvalue, :doc =>
+Puppet::Parser::Functions.newfunction(:fqdn_rand, :arity => -2, :type => :rvalue, :doc =>
   "Usage: `fqdn_rand(MAX, [SEED], [DOWNCASE])`. MAX is required and must be a positive
   integer; SEED is optional and may be any number or string; DOWNCASE is optional
   and should be a boolean true or false.