Merge pull request #19084 from Homebrew/add-comments-to-rubocop-disables

issyl0 · web-flow · commit 4c9de7d31c6d · 2025-01-13T11:32:31.000Z
Add clarifying comments to `rubocop:disable`s
diff --git a/Library/Homebrew/brew.rb b/Library/Homebrew/brew.rb
@@ -200,6 +200,7 @@
   end
 
   exit 1
+# Catch any other types of exceptions.
 rescue Exception => e # rubocop:disable Lint/RescueException
   onoe e
 
diff --git a/Library/Homebrew/build.rb b/Library/Homebrew/build.rb
@@ -230,6 +230,8 @@ def fixopt(formula)
   options = Options.create(args.flags_only)
   build   = Build.new(formula, options, args:)
   build.install
+# Any exception means the build did not complete.
+# The `case` for what to do per-exception class is further down.
 rescue Exception => e # rubocop:disable Lint/RescueException
   error_hash = JSON.parse e.to_json
 
diff --git a/Library/Homebrew/cask/dsl/depends_on.rb b/Library/Homebrew/cask/dsl/depends_on.rb
@@ -63,6 +63,7 @@ def macos=(*args)
             MacOSRequirement.new([T.must(md[:version]).to_sym], comparator: md[:comparator])
           elsif (md = /^\s*(?<comparator><|>|[=<>]=)\s*(?<version>\S+)\s*$/.match(first_arg))
             MacOSRequirement.new([md[:version]], comparator: md[:comparator])
+          # This is not duplicate of the first case: see `args.first` and a different comparator.
           else # rubocop:disable Lint/DuplicateBranch
             MacOSRequirement.new([args.first], comparator: "==")
           end
diff --git a/Library/Homebrew/cmd/update-report.rb b/Library/Homebrew/cmd/update-report.rb
@@ -626,6 +626,7 @@ def migrate_tap_migration
           unless Formulary.factory(new_full_name).keg_only?
             system HOMEBREW_BREW_FILE, "link", new_full_name, "--overwrite"
           end
+        # Rescue any possible exception types.
         rescue Exception => e # rubocop:disable Lint/RescueException
           if Homebrew::EnvConfig.developer?
             require "utils/backtrace"
diff --git a/Library/Homebrew/dev-cmd/test.rb b/Library/Homebrew/dev-cmd/test.rb
@@ -99,6 +99,7 @@ def run
                 exec(*exec_args)
               end
             end
+          # Rescue any possible exception types.
           rescue Exception => e # rubocop:disable Lint/RescueException
             retry if retry_test?(f)
 
diff --git a/Library/Homebrew/download_queue.rb b/Library/Homebrew/download_queue.rb
@@ -19,7 +19,7 @@ def initialize(size = 1)
     sig { params(downloadable: Downloadable, force: T::Boolean).returns(Concurrent::Promises::Future) }
     def enqueue(downloadable, force: false)
       quiet = pool.max_length > 1
-      # Passing in arguments from outside into the future is a common  `concurrent-ruby` pattern.
+      # Passing in arguments from outside into the future is a common `concurrent-ruby` pattern.
       # rubocop:disable Lint/ShadowingOuterLocalVariable
       Concurrent::Promises.future_on(pool, downloadable, force, quiet) do |downloadable, force, quiet|
         downloadable.clear_cache if force
diff --git a/Library/Homebrew/download_strategy.rbi b/Library/Homebrew/download_strategy.rbi
@@ -1,13 +1,14 @@
 # typed: strict
 
-# This is a third-party implementation
+# This is a third-party implementation.
 # rubocop:disable Lint/StructNewOverride
 class Mechanize::HTTP
   ContentDisposition = Struct.new :type, :filename, :creation_date,
                                   :modification_date, :read_date, :size, :parameters
 end
 # rubocop:enable Lint/StructNewOverride
 
+# This is a third-party implementation.
 # rubocop:disable Style/OptionalBooleanParameter
 class Mechanize::HTTP::ContentDispositionParser
   sig {
diff --git a/Library/Homebrew/extend/os/mac/readall.rb b/Library/Homebrew/extend/os/mac/readall.rb
@@ -31,6 +31,7 @@ def valid_casks?(tap, os_name: nil, arch: ::Hardware::CPU.type)
           raise "Missing URL" if cask.url.nil?
         rescue Interrupt
           raise
+        # Handle all possible exceptions reading Casks.
         rescue Exception => e # rubocop:disable Lint/RescueException
           os_and_arch = "macOS #{current_macos_version} on #{arch}"
           onoe "Invalid cask (#{os_and_arch}): #{file}"
diff --git a/Library/Homebrew/formula.rb b/Library/Homebrew/formula.rb
@@ -2903,6 +2903,7 @@ def run_test(keep_tmp: false)
             test
           end
         end
+      # Handle all possible exceptions running formula tests.
       rescue Exception # rubocop:disable Lint/RescueException
         staging.retain! if debug?
         raise
diff --git a/Library/Homebrew/formula_installer.rb b/Library/Homebrew/formula_installer.rb
@@ -484,8 +484,8 @@ def install
     if pour_bottle?
       begin
         pour
+      # Catch any other types of exceptions as they leave us with nothing installed.
       rescue Exception # rubocop:disable Lint/RescueException
-        # any exceptions must leave us with nothing installed
         ignore_interrupts do
           begin
             FileUtils.rm_r(formula.prefix) if formula.prefix.directory?
@@ -825,6 +825,7 @@ def install_dependency(dep, inherited_options)
     oh1 "Installing #{formula.full_name} dependency: #{Formatter.identifier(dep.name)}"
     fi.install
     fi.finish
+  # Handle all possible exceptions installing deps.
   rescue Exception => e # rubocop:disable Lint/RescueException
     ignore_interrupts do
       tmp_keg.rename(installed_keg.to_path) if tmp_keg && !installed_keg.directory?
@@ -1022,6 +1023,7 @@ def build
     formula.update_head_version
 
     raise "Empty installation" if !formula.prefix.directory? || Keg.new(formula.prefix).empty_installation?
+  # Handle all possible exceptions when building.
   rescue Exception => e # rubocop:disable Lint/RescueException
     if e.is_a? BuildError
       e.formula = formula
@@ -1099,6 +1101,7 @@ def link(keg)
       puts "You can try again using:"
       puts "  brew link #{formula.name}"
       @show_summary_heading = true
+    # Handle all other possible exceptions when linking.
     rescue Exception => e # rubocop:disable Lint/RescueException
       ofail "An unexpected error occurred during the `brew link` step"
       puts "The formula built, but is not symlinked into #{HOMEBREW_PREFIX}"
@@ -1151,6 +1154,7 @@ def install_service
     launchd_service_path.chmod 0644
     log = formula.var/"log"
     log.mkpath if service.include? log.to_s
+  # Handle all possible exceptions when installing service files.
   rescue Exception => e # rubocop:disable Lint/RescueException
     puts e
     ofail "Failed to install service files"
@@ -1162,6 +1166,7 @@ def install_service
   sig { params(keg: Keg).void }
   def fix_dynamic_linkage(keg)
     keg.fix_dynamic_linkage
+  # Rescue all possible exceptions when fixing linkage.
   rescue Exception => e # rubocop:disable Lint/RescueException
     ofail "Failed to fix install linkage"
     puts "The formula built, but you may encounter issues using it or linking other"
@@ -1177,6 +1182,7 @@ def fix_dynamic_linkage(keg)
   def clean
     ohai "Cleaning" if verbose?
     Cleaner.new(formula).clean
+  # Handle all possible exceptions when cleaning does not complete.
   rescue Exception => e # rubocop:disable Lint/RescueException
     opoo "The cleaning step did not complete successfully"
     puts "Still, the installation was successful, so we will link it into your prefix."
@@ -1249,6 +1255,7 @@ def post_install
         exec(*args)
       end
     end
+  # Handle all possible exceptions when postinstall does not complete.
   rescue Exception => e # rubocop:disable Lint/RescueException
     opoo "The post-install step did not complete successfully"
     puts "You can try again using:"
diff --git a/Library/Homebrew/ignorable.rb b/Library/Homebrew/ignorable.rb
@@ -25,6 +25,7 @@ def self.hook_raise
       def raise(*)
         callcc do |continuation|
           super
+        # Handle all possible exceptions.
         rescue Exception => e # rubocop:disable Lint/RescueException
           unless e.is_a?(ScriptError)
             e.extend(ExceptionMixin)
diff --git a/Library/Homebrew/migrator.rb b/Library/Homebrew/migrator.rb
@@ -223,6 +223,7 @@ def migrate
     EOS
   rescue Interrupt
     ignore_interrupts { backup_oldname }
+  # Any exception means the migration did not complete.
   rescue Exception => e # rubocop:disable Lint/RescueException
     onoe "The migration did not complete successfully."
     puts e
@@ -357,6 +358,7 @@ def link_newname
       puts
       puts "You can try again using:"
       puts "  brew link #{formula.name}"
+    # Any exception means the `brew link` step did not complete.
     rescue Exception => e # rubocop:disable Lint/RescueException
       onoe "An unexpected error occurred during linking"
       puts e
diff --git a/Library/Homebrew/postinstall.rb b/Library/Homebrew/postinstall.rb
@@ -27,6 +27,7 @@
     formula.extend(Debrew::Formula)
   end
   formula.run_post_install
+# Handle all possible exceptions.
 rescue Exception => e # rubocop:disable Lint/RescueException
   error_pipe.puts e.to_json
   error_pipe.close
diff --git a/Library/Homebrew/readall.rb b/Library/Homebrew/readall.rb
@@ -74,6 +74,7 @@ def self.valid_formulae?(tap, bottle_tag: nil)
       end
     rescue Interrupt
       raise
+    # Handle all possible exceptions reading formulae.
     rescue Exception => e # rubocop:disable Lint/RescueException
       onoe "Invalid formula (#{bottle_tag}): #{file}"
       $stderr.puts e
diff --git a/Library/Homebrew/reinstall.rb b/Library/Homebrew/reinstall.rb
@@ -70,6 +70,7 @@ def self.reinstall_formula(
       fi.finish
     rescue FormulaInstallationAlreadyAttemptedError
       nil
+    # Any other exceptions we want to restore the previous keg and report the error.
     rescue Exception # rubocop:disable Lint/RescueException
       ignore_interrupts { restore_backup(keg, link_keg, verbose:) }
       raise
diff --git a/Library/Homebrew/resource_auditor.rb b/Library/Homebrew/resource_auditor.rb
@@ -91,7 +91,8 @@ def audit_download_strategy
 
     def audit_checksum
       return if spec_name == :head
-      # rubocop:disable Style/InvertibleUnlessCondition (non-invertible)
+      # This condition is non-invertible.
+      # rubocop:disable Style/InvertibleUnlessCondition
       return unless DownloadStrategyDetector.detect(url, using) <= CurlDownloadStrategy
       # rubocop:enable Style/InvertibleUnlessCondition
 
diff --git a/Library/Homebrew/tap.rb b/Library/Homebrew/tap.rb
@@ -1061,6 +1061,7 @@ def self.all
     cache[:all] ||= begin
       core_taps = [
         CoreTap.instance,
+        # The conditional is valid here because we only want the cask tap on macOS.
         (CoreCaskTap.instance if OS.mac?), # rubocop:disable Homebrew/MoveToExtendOS
       ].compact
 
diff --git a/Library/Homebrew/test.rb b/Library/Homebrew/test.rb
@@ -53,6 +53,7 @@
     timeout = ENV["HOMEBREW_TEST_TIMEOUT_SECS"]&.to_i || DEFAULT_TEST_TIMEOUT_SECONDS
     Timeout.timeout(timeout, &run_test)
   end
+# Any exceptions during the test run are reported.
 rescue Exception => e # rubocop:disable Lint/RescueException
   error_pipe.puts e.to_json
   error_pipe.close
diff --git a/Library/Homebrew/test/commands_spec.rb b/Library/Homebrew/test/commands_spec.rb
@@ -2,6 +2,7 @@
 
 require "commands"
 
+# These shared contexts starting with `when` don't make sense.
 RSpec.shared_context "custom internal commands" do # rubocop:disable RSpec/ContextWording
   let(:cmds) do
     [
diff --git a/Library/Homebrew/test/macos_version_spec.rb b/Library/Homebrew/test/macos_version_spec.rb
@@ -22,6 +22,7 @@
   specify "comparison with Symbol" do
     expect(version).to be > :high_sierra
     expect(version).to eq :mojave
+    # We're explicitly testing the `===` operator results here.
     expect(version).to be === :mojave # rubocop:disable Style/CaseEquality
     expect(version).to be < :catalina
   end
@@ -34,13 +35,15 @@
   specify "comparison with String" do
     expect(version).to be > "10.3"
     expect(version).to eq "10.14"
+    # We're explicitly testing the `===` operator results here.
     expect(version).to be === "10.14" # rubocop:disable Style/CaseEquality
     expect(version).to be < "10.15"
   end
 
   specify "comparison with Version" do
     expect(version).to be > Version.new("10.3")
     expect(version).to eq Version.new("10.14")
+    # We're explicitly testing the `===` operator results here.
     expect(version).to be === Version.new("10.14") # rubocop:disable Style/CaseEquality
     expect(version).to be < Version.new("10.15")
   end
diff --git a/Library/Homebrew/test/rubocops/patches_spec.rb b/Library/Homebrew/test/rubocops/patches_spec.rb
@@ -67,6 +67,7 @@ def patches
           expect_offense_hash(message: <<~EOS.chomp, severity: :convention, line: 5, column: 4, source:)
             FormulaAudit/Patches: Patches from Debian should be https://, not http: #{patch_url}
           EOS
+        # GitHub patch diff regexps can't be any shorter.
         # rubocop:disable Layout/LineLength
         elsif patch_url.match?(%r{https?://patch-diff\.githubusercontent\.com/raw/(.+)/(.+)/pull/(.+)\.(?:diff|patch)})
           # rubocop:enable Layout/LineLength
@@ -232,6 +233,7 @@ class Foo < Formula
           expect_offense_hash(message: <<~EOS.chomp, severity: :convention, line: 5, column: 8, source:)
             FormulaAudit/Patches: GitLab patches should end with .diff, not .patch: #{patch_url}
           EOS
+        # GitHub patch diff regexps can't be any shorter.
         # rubocop:disable Layout/LineLength
         elsif patch_url.match?(%r{https?://patch-diff\.githubusercontent\.com/raw/(.+)/(.+)/pull/(.+)\.(?:diff|patch)})
           # rubocop:enable Layout/LineLength
diff --git a/Library/Homebrew/test/support/helper/spec/shared_context/homebrew_cask.rb b/Library/Homebrew/test/support/helper/spec/shared_context/homebrew_cask.rb
@@ -31,6 +31,7 @@ class Config
   end
 end
 
+# These shared contexts starting with `when` don't make sense.
 RSpec.shared_context "Homebrew Cask", :needs_macos do # rubocop:disable RSpec/ContextWording
   around do |example|
     third_party_tap = Tap.fetch("third-party", "tap")
diff --git a/Library/Homebrew/test/support/helper/spec/shared_context/integration_test.rb b/Library/Homebrew/test/support/helper/spec/shared_context/integration_test.rb
@@ -6,6 +6,7 @@
 
 RSpec::Matchers.define_negated_matcher :be_a_failure, :be_a_success
 
+# These shared contexts starting with `when` don't make sense.
 RSpec.shared_context "integration test" do # rubocop:disable RSpec/ContextWording
   extend RSpec::Matchers::DSL
 
diff --git a/Library/Homebrew/test/utils/inreplace_spec.rb b/Library/Homebrew/test/utils/inreplace_spec.rb
@@ -33,6 +33,7 @@
     it "raises error if there is nothing to replace in block form" do
       expect do
         described_class.inreplace(file.path) do |s|
+          # Using `gsub!` here is what we want, and it's only a test.
           s.gsub!("d", "f") # rubocop:disable Performance/StringReplacement
         end
       end.to raise_error(Utils::Inreplace::Error)
diff --git a/Library/Homebrew/test/utils/spdx_spec.rb b/Library/Homebrew/test/utils/spdx_spec.rb
@@ -75,6 +75,7 @@
       license_expression = { any_of: [
         "MIT",
         :public_domain,
+        # The final array item is legitimately a hash in the case of license expressions.
         all_of: ["0BSD", "Zlib"], # rubocop:disable Style/HashAsLastArrayItem
         "curl" => { with: "LLVM-exception" },
       ] }
@@ -195,6 +196,7 @@
       license_expression = { any_of: [
         "MIT",
         :public_domain,
+        # The final array item is legitimately a hash in the case of license expressions.
         all_of: ["0BSD", "Zlib"], # rubocop:disable Style/HashAsLastArrayItem
         "curl" => { with: "LLVM-exception" },
       ] }
@@ -235,6 +237,7 @@
       })
     end
 
+    # The final array item is legitimately a hash in the case of license expressions.
     # rubocop:disable Style/HashAsLastArrayItem
     it "handles nested brackets" do
       expect(described_class.string_to_license_expression("A AND (B OR (C AND D))")).to eq({
diff --git a/Library/Homebrew/test/utils/string_inreplace_extension_spec.rb b/Library/Homebrew/test/utils/string_inreplace_extension_spec.rb
@@ -269,6 +269,7 @@
     let(:string) { "foo" }
 
     it "replaces all occurrences" do
+      # Using `gsub!` here is what we want, and it's only a test.
       string_extension.gsub!("o", "e") # rubocop:disable Performance/StringReplacement
       expect(string_extension.inreplace_string).to eq("fee")
     end
diff --git a/Library/Homebrew/utils/fork.rb b/Library/Homebrew/utils/fork.rb
@@ -52,6 +52,7 @@ def self.safe_fork(directory: nil, yield_parent: false)
           Process::UID.change_privilege(Process.euid) if Process.euid != Process.uid
 
           yield(error_pipe)
+        # This could be any type of exception, so rescue them all.
         rescue Exception => e # rubocop:disable Lint/RescueException
           error_hash = JSON.parse e.to_json
 
