Clean up rubocop configuration pt.1 (#11566)

.pre-commit-config.yaml

@@ -3,7 +3,7 @@
 # These pre-commit hooks mirror the Github CI, so running them locally will catch errors earlier.
 repos:
   - repo: https://github.com/rubocop/rubocop
-    rev: v1.64.1
+    rev: v1.74.0
     hooks:
       - id: rubocop
   - repo: https://github.com/syntaqx/git-hooks
@@ -17,6 +17,6 @@ repos:
       - id: markdownlint
         args: [-s.mdl_style.rb]
   - repo: https://github.com/adrienverge/yamllint
-    rev: v1.35.1
+    rev: v1.36.1
     hooks:
       - id: yamllint

.rubocop.yml

@@ -22,9 +22,7 @@ Lint/OrAssignmentToConstant:
     - 'lib/const.rb'
     - 'lib/fixup.rb'
 
-Lint/SuppressedException:
-  Enabled: false
-
+# TODO
 Layout/HashAlignment:
   EnforcedHashRocketStyle:
     - separator
@@ -35,83 +33,89 @@ Layout/HashAlignment:
 
 # End temporarily disabled cops
 
+# These cops have either been disabled permanently or have a non-default configuration option.
+# If you permanently disable another cop, please add a comment explaining why.
+# If you set a non-default configuration for a cop, please add a comment explaining why.
+
+# An empty line after a guard clause can improve readability, but mandating it reduces compactness.
 Layout/EmptyLineAfterGuardClause:
   Enabled: false
 
+# TODO
 Layout/FirstHashElementIndentation:
   Enabled: false
 # EnforcedStyle: consistent
 
+# A maximum line length tends to impede development more than it helps readability.
 Layout/LineLength:
   Enabled: false
 
+# Having an explicit return can often improve readability.
 Style/RedundantReturn:
   Enabled: false
 
+# A comment at the top of every file is not worth the minor benefits of frozen strings.
+# Besides, Ruby is in the process of freezing strings by default anyway.
 Style/FrozenStringLiteralComment:
   Enabled: false
 
+# Explicitly freezing every constant is too verbose, and provides minor benefits regardless.
 Style/MutableConstant:
   Enabled: false
 
+# Mandating top-level documentation comments and the like will more often than not lead to meaningless documentation.
+# Let people choose what does and does not need to be documented.
 Style/Documentation:
   Enabled: false
 
+# While using a guard clause reduces indentation and lines of code, it is not always the most logical flow.
+# Let people choose which is best depending on the scenario.
 Style/GuardClause:
   Enabled: false
 
-Style/DocumentDynamicEvalDefinition:
-  Enabled: false
-
-# https://github.com/rubocop/rubocop/issues/12132
-Style/OptionalBooleanParameter:
-  Enabled: false
-
+# TODO
 Style/ClassVars:
   Enabled: false
 
+# TODO
 Style/GlobalVars:
   Enabled: false
 
+# TODO
 Style/FormatStringToken:
   Enabled: false
 
+# TODO
 Style/MultilineBlockChain:
   Enabled: false
 
+# The only numeric literals in the codebase long enough to trip this are kilobyte sizes, for which a thousands separator does not improve readability.
 Style/NumericLiterals:
   Enabled: false
 
-Style/RedundantCapitalW:
-  Enabled: false
-
-Style/RedundantLineContinuation:
-  Enabled: false
-
+# TODO
 Style/RedundantStringEscape:
   Enabled: false
 
+# Enabling Metrics results in offenses that essentially mandate the arbitrary splitting of logic into additional files and methods.
+# Even the rubocop codebase itself often inline disables these offfenses, so lets save the time and disable them all here.
 Metrics:
   Enabled: false
 
-Naming/AccessorMethodName:
-  Enabled: false
-
-Naming/BlockParameterName:
-  Enabled: false
-
+# We disable this for packages because we don't want to incorrectly convert things like Xz_java into XzJava.
+# Many package names include - or _ and we don't want to lose that information by mistakenly converting into CamelCase.
 Naming/ClassAndModuleCamelCase:
   Exclude:
     - 'packages/*'
 
+# We allow all caps variables to mirror our constants, so things like @ARCH_LDFLAGS are allowed.
 Naming/VariableName:
   EnforcedStyle: snake_case
   # Allow variable names which are in all caps (with optional numbers and underscores)
   AllowedPatterns:
     - '^@?[A-Z0-9_]+$'
 
-Naming/VariableNumber:
-  Enabled: false
-
+# If there is meaningful information to convey about the contents of a heredoc, a comment is the best way to do it.
+# Forcing unique names for all heredoc delimiters does not improve documentation in the vast majority of cases.
 Naming/HeredocDelimiterNaming:
   Enabled: false

bin/crew

@@ -139,7 +139,7 @@ at_exit do
   ExitMessage.print
 end
 
-def print_current_package(extra = false)
+def print_current_package(extra: false)
   status = if PackageUtils.installed?(@pkg.name)
              :installed
            elsif !PackageUtils.compatible?(@pkg)
@@ -167,7 +167,7 @@ def print_current_package(extra = false)
   puts ''
 end
 
-def set_package(pkg_path)
+def check_load_package(pkg_path)
   begin
     @pkg = Package.load_package(pkg_path)
   rescue SyntaxError => e
@@ -179,7 +179,7 @@ end
 
 def search(pkg_name, pkg_path: File.join(CREW_PACKAGES_PATH, "#{pkg_name}.rb"), silent: false)
   begin
-    return set_package(pkg_path) if File.file?(pkg_path)
+    return check_load_package(pkg_path) if File.file?(pkg_path)
   rescue StandardError => e
     puts "Error with #{pkg_name}.rb: #{e}".lightred unless e.to_s.include?('uninitialized constant')
   end
@@ -486,7 +486,7 @@ def download
         end
       end
       # Download file if not cached.
-      downloader url, sha256sum, filename, CREW_VERBOSE
+      downloader url, sha256sum, filename, verbose: CREW_VERBOSE
 
       puts "#{@pkg.name.capitalize} archive downloaded.".lightgreen
       # Stow file in cache if requested, if file is not from cache,
@@ -513,7 +513,7 @@ def download
     else
       unless git # We don't want to download a git repository as a file.
         FileUtils.mkdir_p @extract_dir
-        downloader url, sha256sum, filename, CREW_VERBOSE
+        downloader url, sha256sum, filename, verbose: CREW_VERBOSE
 
         puts "#{filename}: File downloaded.".lightgreen
 
@@ -1625,7 +1625,7 @@ def build_command(args)
     next unless Command.check(name, @opt_force)
 
     search @pkg_name
-    print_current_package CREW_VERBOSE
+    print_current_package extra: CREW_VERBOSE
     next unless @pkg_name
 
     # Process preflight block to see if package should be built
@@ -1680,7 +1680,7 @@ def download_command(args)
     @pkg_name = name
     search @pkg_name
     @pkg.build_from_source = true if @opt_source
-    print_current_package CREW_VERBOSE
+    print_current_package extra: CREW_VERBOSE
     if ARGV.intersect?(%w[download]) && @pkg.is_fake?
       fake_pkg_deplist = @pkg.get_deps_list(return_attr: true).flat_map(&:keys).uniq
       until fake_pkg_deplist.blank?
@@ -1731,7 +1731,7 @@ def install_command(args)
     end
     next unless Command.check(name, @opt_force)
     search @pkg_name
-    print_current_package true
+    print_current_package extra: true
     @pkg.build_from_source = true if @opt_source || @opt_recursive || CREW_BUILD_FROM_SOURCE
     next unless @pkg_name
 

lib/const.rb

@@ -3,7 +3,7 @@
 require 'etc'
 
 OLD_CREW_VERSION ||= defined?(CREW_VERSION) ? CREW_VERSION : '1.0'
-CREW_VERSION ||= '1.57.8' unless defined?(CREW_VERSION) && CREW_VERSION == OLD_CREW_VERSION
+CREW_VERSION ||= '1.57.9' unless defined?(CREW_VERSION) && CREW_VERSION == OLD_CREW_VERSION
 
 # Kernel architecture.
 KERN_ARCH ||= Etc.uname[:machine]
@@ -61,7 +61,7 @@ else
 end
 
 # Use sane minimal defaults if in container and no override specified.
-CREW_KERNEL_VERSION ||= \
+CREW_KERNEL_VERSION ||=
   if CREW_IN_CONTAINER && ENV.fetch('CREW_KERNEL_VERSION', nil).nil?
     ARCH.eql?('i686') ? '3.8' : '5.10'
   else
@@ -106,7 +106,7 @@ CREW_FORCE   ||= ARGV.intersect?(%w[-f --force]) unless defined?(CREW_FORCE)
 CREW_VERBOSE ||= ARGV.intersect?(%w[-v --verbose]) unless defined?(CREW_VERBOSE)
 
 # Set CREW_NPROC from environment variable, `distcc -j`, or `nproc`.
-CREW_NPROC ||= \
+CREW_NPROC ||=
   if File.file?("#{CREW_PREFIX}/bin/distcc")
     ENV.fetch('CREW_NPROC', `distcc -j`.chomp)
   else
@@ -139,7 +139,7 @@ CREW_BRANCH ||= ENV.fetch('CREW_BRANCH', 'master') unless defined?(CREW_BRANCH)
 USER ||= Etc.getlogin unless defined?(USER)
 
 unless defined?(CHROMEOS_RELEASE)
-  CHROMEOS_RELEASE = \
+  CHROMEOS_RELEASE =
     if File.exist?('/etc/lsb-release')
       File.read('/etc/lsb-release')[/CHROMEOS_RELEASE_CHROME_MILESTONE||=(.+)/, 1]
     else
@@ -168,7 +168,7 @@ CREW_DOWNLOADER_RETRY ||= ENV.fetch('CREW_DOWNLOADER_RETRY', 3).to_i unless defi
 CREW_HIDE_PROGBAR = ENV.fetch('CREW_HIDE_PROGBAR', false) unless defined?(CREW_HIDE_PROGBAR)
 
 # set certificate file location for lib/downloader.rb
-SSL_CERT_FILE ||= \
+SSL_CERT_FILE ||=
   if ENV['SSL_CERT_FILE'] && File.exist?(ENV['SSL_CERT_FILE'])
     ENV['SSL_CERT_FILE']
   elsif File.exist?("#{CREW_PREFIX}/etc/ssl/certs/ca-certificates.crt")
@@ -177,7 +177,7 @@ SSL_CERT_FILE ||= \
     '/etc/ssl/certs/ca-certificates.crt'
   end
 
-SSL_CERT_DIR ||= \
+SSL_CERT_DIR ||=
   if ENV['SSL_CERT_DIR'] && Dir.exist?(ENV['SSL_CERT_DIR'])
     ENV['SSL_CERT_DIR']
   elsif Dir.exist?("#{CREW_PREFIX}/etc/ssl/certs")
@@ -211,7 +211,7 @@ CREW_COMMON_FNO_LTO_FLAGS ||= "#{CREW_CORE_FLAGS} -fno-lto"
 CREW_LDFLAGS              ||= "-flto=auto #{CREW_LINKER_FLAGS}"
 CREW_FNO_LTO_LDFLAGS      ||= '-fno-lto'
 
-CREW_ENV_OPTIONS_HASH ||= \
+CREW_ENV_OPTIONS_HASH ||=
   if CREW_DISABLE_ENV_OPTIONS
     { 'CREW_DISABLE_ENV_OPTIONS' => '1' }
   else

lib/convenience_functions.rb

@@ -115,7 +115,7 @@ class ConvenienceFunctions
 
   def self.unset_default_browser(browser_name, browser_binary)
     Dir.chdir("#{CREW_PREFIX}/bin") do
-      if File.exist?('x-www-browser') && File.symlink?('x-www-browser') && \
+      if File.exist?('x-www-browser') && File.symlink?('x-www-browser') &&
          File.realpath('x-www-browser') == "#{CREW_PREFIX}/share/#{browser_name.downcase}/#{browser_binary}"
         FileUtils.rm "#{CREW_PREFIX}/bin/x-www-browser"
       end

lib/deb_utils.rb

@@ -29,7 +29,7 @@ module DebUtils
       end
 
       # read file meta
-      name, _modtime, _uid, _gid, mode, size, end_char = \
+      name, _modtime, _uid, _gid, mode, size, end_char =
         line.scan(/(.{16})(.{12})(.{6})(.{6})(.{8})(.{10})(.{1})/).flatten.map(&:strip)
 
       # remove slash suffix from filename (if any)

lib/downloader.rb

@@ -24,7 +24,7 @@ rescue RuntimeError => e
   end
 end
 
-def downloader(url, sha256sum, filename = File.basename(url), verbose = false)
+def downloader(url, sha256sum, filename = File.basename(url), verbose: false)
   # downloader: wrapper for all Chromebrew downloaders (`net/http`,`curl`...)
   # Usage: downloader <url>, <sha256sum>, <filename::optional>, <verbose::optional>
   #
@@ -39,13 +39,13 @@ def downloader(url, sha256sum, filename = File.basename(url), verbose = false)
   if CREW_USE_CURL || !ENV['CREW_DOWNLOADER'].to_s.empty?
     # force using external downloader if either CREW_USE_CURL or ENV['CREW_DOWNLOADER'] is set
     puts "external_downloader(#{uri}, #{filename}, #{verbose})" if verbose
-    external_downloader(uri, filename, verbose)
+    external_downloader(uri, filename, verbose: verbose)
   else
     case uri.scheme
     when 'http', 'https'
       # use net/http if the url protocol is http(s)://
       puts "http_downloader(#{uri}, #{filename}, #{verbose})" if verbose
-      http_downloader(uri, filename, verbose)
+      http_downloader(uri, filename, verbose: verbose)
     when 'file'
       # use FileUtils to copy if it is a local file (the url protocol is file://)
       if File.exist?(uri.path)
@@ -56,7 +56,7 @@ def downloader(url, sha256sum, filename = File.basename(url), verbose = false)
     else
       # use external downloader (curl by default) if the url protocol is not http(s):// or file://
       puts "external_downloader(#{uri}, #{filename}, #{verbose})" if verbose
-      external_downloader(uri, filename, verbose)
+      external_downloader(uri, filename, verbose: verbose)
     end
   end
 
@@ -88,10 +88,10 @@ rescue StandardError => e
 
   # fallback to curl if error occurred
   puts "external_downloader(#{uri}, #{filename}, #{verbose})" if verbose
-  external_downloader(uri, filename, verbose)
+  external_downloader(uri, filename, verbose: verbose)
 end
 
-def http_downloader(uri, filename = File.basename(url), verbose = false)
+def http_downloader(uri, filename = File.basename(url), verbose: false)
   # http_downloader: Downloader based on net/http library
   ssl_error_retry = 0
 
@@ -119,7 +119,7 @@ def http_downloader(uri, filename = File.basename(url), verbose = false)
         redirect_uri.scheme ||= uri.scheme
         redirect_uri.host ||= uri.host
 
-        return send(__method__, redirect_uri, filename, verbose)
+        return send(__method__, redirect_uri, filename, verbose: verbose)
       else
         abort "Download of #{uri} failed with error #{response.code}: #{response.msg}".lightred
       end
@@ -167,7 +167,7 @@ rescue OpenSSL::SSL::SSLError
   ssl_error_retry <= 3 ? retry : raise
 end
 
-def external_downloader(uri, filename = File.basename(url), verbose = false)
+def external_downloader(uri, filename = File.basename(url), verbose: false)
   # external_downloader: wrapper for external downloaders in CREW_DOWNLOADER (curl by default)
 
   # default curl cmdline, CREW_DOWNLOADER should be in this format also

lib/package.rb

@@ -143,9 +143,9 @@ class Package
     deps = pkg_obj.dependencies
 
     # Append buildessential to deps if building from source is needed/specified.
-    if ((include_build_deps == true) || ((include_build_deps == 'auto') && is_source)) && \
-       !pkg_obj.no_compile_needed? && \
-       !exclude_buildessential && \
+    if ((include_build_deps == true) || ((include_build_deps == 'auto') && is_source)) &&
+       !pkg_obj.no_compile_needed? &&
+       !exclude_buildessential &&
        !@checked_list.keys.include?('buildessential')
 
       deps = { 'buildessential' => [[:build]] }.merge(deps)
@@ -155,8 +155,8 @@ class Package
     expanded_deps = deps.uniq.map do |dep, (dep_tags, ver_check)|
       # Check build dependencies only if building from source is needed/specified.
       # Do not recursively find :build based build dependencies.
-      next unless (include_build_deps == true && @crew_current_package == pkg_obj.name) || \
-                  ((include_build_deps == 'auto') && is_source && @crew_current_package == pkg_obj.name) || \
+      next unless (include_build_deps == true && @crew_current_package == pkg_obj.name) ||
+                  ((include_build_deps == 'auto') && is_source && @crew_current_package == pkg_obj.name) ||
                   !dep_tags.include?(:build)
 
       # Overwrite tags if parent dependency is a build dependency.

lib/selector.rb

@@ -94,6 +94,7 @@ class Selector
       # discard any input in the input buffer
       $stdin.read_nonblock(1024)
     rescue IO::WaitReadable
+      # We wait here for reading as per https://docs.ruby-lang.org/en/master/IO.html#method-c-select
     ensure
       Thread.current[:input] = $stdin.getc
     end

packages/firefox.rb

@@ -103,7 +103,7 @@ class Firefox < Package
 
   def self.preremove
     Dir.chdir("#{CREW_PREFIX}/bin") do
-      if File.exist?('x-www-browser') && File.symlink?('x-www-browser') && \
+      if File.exist?('x-www-browser') && File.symlink?('x-www-browser') &&
          File.realpath('x-www-browser') == "#{CREW_PREFIX}/bin/firefox"
         FileUtils.rm "#{CREW_PREFIX}/bin/x-www-browser"
       end

packages/foremost.rb

@@ -22,8 +22,8 @@ class Foremost < Package
   end
 
   def self.install
-    FileUtils.mkdir_p ["#{CREW_DEST_PREFIX}/bin", \
-                       "#{CREW_DEST_PREFIX}/man/man1", \
+    FileUtils.mkdir_p ["#{CREW_DEST_PREFIX}/bin",
+                       "#{CREW_DEST_PREFIX}/man/man1",
                        "#{CREW_DEST_PREFIX}/etc"]
     FileUtils.cp_r 'foremost', "#{CREW_DEST_PREFIX}/bin/"
     FileUtils.cp_r 'foremost.8.gz', "#{CREW_DEST_PREFIX}/man/man1/foremost.1.gz"

packages/nss.rb

@@ -26,12 +26,12 @@ class Nss < Package
   depends_on 'zlib' # R
 
   def self.build
-    @build_64 = ARCH == 'x86_64' ? '1' : '0'
+    @build64 = ARCH == 'x86_64' ? '1' : '0'
     @arch_cflags = ''
     @arch_ldflags = @arch_cflags
 
     Dir.chdir 'nss' do
-      system "env opt_build=1 build_64=#{@build_64} \
+      system "env opt_build=1 build_64=#{@build64} \
         NSS_ENABLE_WERROR=0 NS_USE_GCC=1 USEABSPATH=NO \
         NSS_GYP_PREFIX=#{CREW_PREFIX} CFLAGS='-pipe #{@arch_cflags}' \
         CXXFLAGS='-pipe #{@arch_cflags}' LDFLAGS='#{@arch_ldflags}' \

packages/qemacs.rb

@@ -20,12 +20,12 @@ class Qemacs < Package
   def self.build
     system "sed -i 's,css.h,libqhtml/css.h,' html2png.c"
     system "sed -i 's/$(prefix)/$(DESTDIR)$(prefix)/g' Makefile"
-    system './configure', \
-           "--prefix=#{CREW_PREFIX}", \
-           '--disable-x11', \
-           '--disable-xv', \
-           '--disable-xrender', \
-           '--disable-html', \
+    system './configure',
+           "--prefix=#{CREW_PREFIX}",
+           '--disable-x11',
+           '--disable-xv',
+           '--disable-xrender',
+           '--disable-html',
            '--disable-png'
     system 'make'
   end

packages/vifm.rb

@@ -18,10 +18,10 @@ class Vifm < Package
   })
 
   def self.build
-    system './configure', \
-           "--prefix=#{CREW_PREFIX}", \
-           '--without-gtk', \
-           '--without-X11', \
+    system './configure',
+           "--prefix=#{CREW_PREFIX}",
+           '--without-gtk',
+           '--without-X11',
            # "--with-curses=#{CREW_PREFIX}/include/ncursesw" \
            # "--with-curses-name=ncursesw" \
            "CPPFLAGS=-I#{CREW_PREFIX}/include/ncursesw"

tools/getrealdeps.rb

@@ -99,9 +99,7 @@ def main(pkg)
   end.flatten.compact.uniq
 
   # Figure out which Chromebrew packages provide the relevant deps.
-  pkgdeps = pkgdepsfiles.map do |file|
-              whatprovidesfxn(file, pkg)
-            end.sort.reject { |i| i.include?(pkg) }.map { |i| i.split("\n") }.flatten.uniq
+  pkgdeps = pkgdepsfiles.map { |file| whatprovidesfxn(file, pkg) }.sort.reject { |i| i.include?(pkg) }.map { |i| i.split("\n") }.flatten.uniq
 
   # Massage the glibc entries in the dependency list.
   pkgdeps = pkgdeps.map { |i| i.gsub(/glibc_build.*/, 'glibc') }.uniq