From df8e97fef60ac71337867d87431e304b78ae32c0 Mon Sep 17 00:00:00 2001
From: Issy Long <me@issyl0.co.uk>
Date: Fri, 31 Mar 2023 01:25:36 +0100
Subject: [PATCH] Consolidate `add_{warning,error}` methods into one

- Specify `strictish: true` in `add_error` to specify that it's not a
  super big critical error.
- These will be shown only if `brew audit --strict` is requested.
---
 Library/Homebrew/cask/audit.rb           | 61 ++++++++++--------------
 Library/Homebrew/test/cask/audit_spec.rb | 30 ++++--------
 2 files changed, 35 insertions(+), 56 deletions(-)

diff --git a/Library/Homebrew/cask/audit.rb b/Library/Homebrew/cask/audit.rb
index aab2793c12..b7bc785c3c 100644
--- a/Library/Homebrew/cask/audit.rb
+++ b/Library/Homebrew/cask/audit.rb
@@ -81,15 +81,12 @@ module Cask
       !errors?
     end
 
-    sig { params(message: T.nilable(String), location: T.nilable(String)).void }
-    def add_error(message, location: nil)
-      errors << ({ message: message, location: location })
-    end
+    sig { params(message: T.nilable(String), location: T.nilable(String), strictish: T::Boolean).void }
+    def add_error(message, location: nil, strictish: false)
+      # Only raise non-critical audits if the user specified `--strict`.
+      return if strictish && !@strict
 
-    sig { params(message: T.nilable(String), location: T.nilable(String)).void }
-    def add_warning(message, location: nil)
-      # Warnings are ignored unless `--strict` is passed in which case they're turned into errors.
-      add_error(message, location: location) if strict?
+      errors << ({ message: message, location: location })
     end
 
     def result
@@ -195,7 +192,7 @@ module Cask
       # increases the maintenance burden.
       return if cask.tap == "homebrew/cask-fonts"
 
-      add_warning "Cask should have a description. Please add a `desc` stanza." if cask.desc.blank?
+      add_error("Cask should have a description. Please add a `desc` stanza.", strictish: true) if cask.desc.blank?
     end
 
     sig { void }
@@ -383,8 +380,10 @@ module Cask
       return unless token_conflicts?
       return unless core_formula_names.include?(cask.token)
 
-      add_warning "possible duplicate, cask token conflicts with Homebrew core formula: " \
-                  "#{Formatter.url(core_formula_url)}"
+      add_error(
+        "possible duplicate, cask token conflicts with Homebrew core formula: #{Formatter.url(core_formula_url)}",
+        strictish: true,
+      )
     end
 
     sig { void }
@@ -418,18 +417,19 @@ module Cask
         add_error "cask token contains version designation '#{match_data[:designation]}'"
       end
 
-      add_warning "cask token mentions launcher" if token.end_with? "launcher"
+      add_error("cask token mentions launcher", strictish: true) if token.end_with? "launcher"
 
-      add_warning "cask token mentions desktop" if token.end_with? "desktop"
+      add_error("cask token mentions desktop", strictish: true) if token.end_with? "desktop"
 
-      add_warning "cask token mentions platform" if token.end_with? "mac", "osx", "macos"
+      add_error("cask token mentions platform", strictish: true) if token.end_with? "mac", "osx", "macos"
 
-      add_warning "cask token mentions architecture" if token.end_with? "x86", "32_bit", "x86_64", "64_bit"
+      add_error("cask token mentions architecture", strictish: true) if token.end_with? "x86", "32_bit", "x86_64",
+                                                                                        "64_bit"
 
       frameworks = %w[cocoa qt gtk wx java]
       return if frameworks.include?(token) || !token.end_with?(*frameworks)
 
-      add_warning "cask token mentions framework"
+      add_error("cask token mentions framework", strictish: true)
     end
 
     sig { void }
@@ -449,7 +449,10 @@ module Cask
       return if cask.url.to_s.include? cask.version.csv.second
       return if cask.version.csv.third.present? && cask.url.to_s.include?(cask.version.csv.third)
 
-      add_warning "Download does not require additional version components. Use `&:short_version` in the livecheck"
+      add_error(
+        "Download does not require additional version components. Use `&:short_version` in the livecheck",
+        strictish: true,
+      )
     end
 
     sig { void }
@@ -493,7 +496,7 @@ module Cask
               "#{message} fix the signature of their app."
             end
 
-            add_warning message
+            add_error(message, strictish: true)
           when Artifact::Pkg
             path = downloaded_path
             next unless path.exist?
@@ -501,7 +504,7 @@ module Cask
             result = system_command("pkgutil", args: ["--check-signature", path], print_stderr: false)
 
             unless result.success?
-              add_warning <<~EOS
+              add_error(<<~EOS, strictish: true)
                 Signature verification failed:
                 #{result.merged_output}
                 macOS on ARM requires applications to be signed.
@@ -513,7 +516,7 @@ module Cask
             result = system_command("stapler", args: ["validate", path], print_stderr: false)
             next if result.success?
 
-            add_warning <<~EOS
+            add_error(<<~EOS, strictish: true)
               Signature verification failed:
               #{result.merged_output}
               macOS on ARM requires applications to be signed.
@@ -638,16 +641,9 @@ module Cask
 
       metadata = SharedAudits.github_repo_data(user, repo)
       return if metadata.nil?
-
       return unless metadata["archived"]
 
-      message = "GitHub repo is archived"
-
-      if cask.discontinued?
-        add_warning message
-      else
-        add_error message
-      end
+      add_error("GitHub repo is archived", strictish: cask.discontinued?)
     end
 
     sig { void }
@@ -659,16 +655,9 @@ module Cask
 
       metadata = SharedAudits.gitlab_repo_data(user, repo)
       return if metadata.nil?
-
       return unless metadata["archived"]
 
-      message = "GitLab repo is archived"
-
-      if cask.discontinued?
-        add_warning message
-      else
-        add_error message
-      end
+      add_error("GitLab repo is archived", strictish: cask.discontinued?)
     end
 
     sig { void }
diff --git a/Library/Homebrew/test/cask/audit_spec.rb b/Library/Homebrew/test/cask/audit_spec.rb
index 873fdb954a..6a32f27030 100644
--- a/Library/Homebrew/test/cask/audit_spec.rb
+++ b/Library/Homebrew/test/cask/audit_spec.rb
@@ -44,16 +44,6 @@ describe Cask::Audit, :cask do
     end
   end
 
-  matcher :warn_with do |message|
-    match do |audit|
-      include_msg?(audit.errors, message)
-    end
-
-    failure_message do |audit|
-      "expected to warn with message #{message.inspect} but #{outcome(audit)}"
-    end
-  end
-
   let(:cask) { instance_double(Cask::Cask) }
   let(:new_cask) { nil }
   let(:online) { nil }
@@ -111,7 +101,7 @@ describe Cask::Audit, :cask do
 
     context "when there are no errors and `--strict` is not passed so we should not show anything" do
       before do
-        audit.add_warning "eh"
+        audit.add_error("eh", strictish: true)
       end
 
       it { is_expected.not_to match(/failed/) }
@@ -128,7 +118,7 @@ describe Cask::Audit, :cask do
     context "when there are errors and warnings" do
       before do
         audit.add_error "bad"
-        audit.add_warning "eh"
+        audit.add_error("eh", strictish: true)
       end
 
       it { is_expected.to match(/failed/) }
@@ -139,7 +129,7 @@ describe Cask::Audit, :cask do
 
       before do
         audit.add_error "very bad"
-        audit.add_warning "a little bit bad"
+        audit.add_error("a little bit bad", strictish: true)
       end
 
       it { is_expected.to match(/failed/) }
@@ -147,7 +137,7 @@ describe Cask::Audit, :cask do
 
     context "when there are warnings and `--strict` is not passed" do
       before do
-        audit.add_warning "a little bit bad"
+        audit.add_error("a little bit bad", strictish: true)
       end
 
       it { is_expected.not_to match(/failed/) }
@@ -157,7 +147,7 @@ describe Cask::Audit, :cask do
       let(:strict) { true }
 
       before do
-        audit.add_warning "a little bit bad"
+        audit.add_error("a little bit bad", strictish: true)
       end
 
       it { is_expected.to match(/failed/) }
@@ -501,7 +491,7 @@ describe Cask::Audit, :cask do
         it "does not fail" do
           expect(download_double).not_to receive(:fetch)
           expect(UnpackStrategy).not_to receive(:detect)
-          expect(run).not_to warn_with(/Audit\.app/)
+          expect(run).not_to error_with(/Audit\.app/)
         end
       end
 
@@ -519,7 +509,7 @@ describe Cask::Audit, :cask do
         it "does not fail since no extract" do
           allow(download_double).to receive(:fetch).and_return(Pathname.new("/tmp/test.zip"))
           allow(UnpackStrategy).to receive(:detect).and_return(nil)
-          expect(run).not_to warn_with(/Audit\.app/)
+          expect(run).not_to error_with(/Audit\.app/)
         end
       end
     end
@@ -1005,14 +995,14 @@ describe Cask::Audit, :cask do
 
           it "warns about duplicates" do
             expect(audit).to receive(:core_formula_names).and_return(formula_names)
-            expect(run).to warn_with(/possible duplicate/)
+            expect(run).to error_with(/possible duplicate/)
           end
         end
 
         context "when `--strict` is not passed" do
           it "does not warn about duplicates" do
             expect(audit).to receive(:core_formula_names).and_return(formula_names)
-            expect(run).not_to warn_with(/possible duplicate/)
+            expect(run).not_to error_with(/possible duplicate/)
           end
         end
       end
@@ -1086,7 +1076,7 @@ describe Cask::Audit, :cask do
         let(:new_cask) { false }
 
         it "does not warn" do
-          expect(run).not_to warn_with(/should have a description/)
+          expect(run).not_to error_with(/should have a description/)
         end
       end