From 2b8127d5181b4f8a7e56b1afc210ec454162829a Mon Sep 17 00:00:00 2001
From: Issy Long <me@issyl0.co.uk>
Date: Thu, 30 Mar 2023 23:52:24 +0100
Subject: [PATCH] Turn cask warnings into errors when `--strict` is passed (or
 implied)

- Ignore them and don't show them otherwise.
- Part three of issue 15074:
  > As a result, I propose that all current cask audit warnings are never
  > displayed as warnings but the underlying audit checks turned into
  > errors displayed only with --strict (or one of the other relevant
  > flags).
---
 Library/Homebrew/cask/audit.rb                | 35 ++------
 Library/Homebrew/cask/auditor.rb              | 13 +--
 Library/Homebrew/cask/cmd/audit.rb            |  1 -
 Library/Homebrew/test/cask/audit_spec.rb      | 81 ++++++++++++-------
 Library/Homebrew/test/cask/quarantine_spec.rb |  8 +-
 5 files changed, 66 insertions(+), 72 deletions(-)

diff --git a/Library/Homebrew/cask/audit.rb b/Library/Homebrew/cask/audit.rb
index c9d299e3e9..aab2793c12 100644
--- a/Library/Homebrew/cask/audit.rb
+++ b/Library/Homebrew/cask/audit.rb
@@ -71,23 +71,14 @@ module Cask
       @errors ||= []
     end
 
-    def warnings
-      @warnings ||= []
-    end
-
     sig { returns(T::Boolean) }
     def errors?
       errors.any?
     end
 
-    sig { returns(T::Boolean) }
-    def warnings?
-      warnings.any?
-    end
-
     sig { returns(T::Boolean) }
     def success?
-      !(errors? || warnings?)
+      !errors?
     end
 
     sig { params(message: T.nilable(String), location: T.nilable(String)).void }
@@ -97,25 +88,17 @@ module Cask
 
     sig { params(message: T.nilable(String), location: T.nilable(String)).void }
     def add_warning(message, location: nil)
-      if strict?
-        add_error message, location: location
-      else
-        warnings << ({ message: message, location: location })
-      end
+      # Warnings are ignored unless `--strict` is passed in which case they're turned into errors.
+      add_error(message, location: location) if strict?
     end
 
     def result
-      if errors?
-        Formatter.error("failed")
-      elsif warnings?
-        Formatter.warning("warning")
-      end
+      Formatter.error("failed") if errors?
     end
 
-    sig { params(include_warnings: T::Boolean).returns(T.nilable(String)) }
-    def summary(include_warnings: true)
+    sig { returns(T.nilable(String)) }
+    def summary
       return if success?
-      return if warnings? && !errors? && !include_warnings
 
       summary = ["audit for #{cask}: #{result}"]
 
@@ -123,12 +106,6 @@ module Cask
         summary << " #{Formatter.error("-")} #{error[:message]}"
       end
 
-      if include_warnings
-        warnings.each do |warning|
-          summary << " #{Formatter.warning("-")} #{warning[:message]}"
-        end
-      end
-
       summary.join("\n")
     end
 
diff --git a/Library/Homebrew/cask/auditor.rb b/Library/Homebrew/cask/auditor.rb
index cae71e5d13..7a744f2172 100644
--- a/Library/Homebrew/cask/auditor.rb
+++ b/Library/Homebrew/cask/auditor.rb
@@ -45,7 +45,6 @@ module Cask
     LANGUAGE_BLOCK_LIMIT = 10
 
     def audit
-      warnings = Set.new
       errors = Set.new
 
       if !language && language_blocks
@@ -59,23 +58,19 @@ module Cask
 
         sample_languages.each_key do |l|
           audit = audit_languages(l)
-          summary = audit.summary(include_warnings: output_warnings?)
-          if summary.present? && output_summary?(audit)
+          if audit.summary.present? && output_summary?(audit)
             ohai "Auditing language: #{l.map { |lang| "'#{lang}'" }.to_sentence}" if output_summary?
-            puts summary
+            puts audit.summary
           end
-          warnings += audit.warnings
           errors += audit.errors
         end
       else
         audit = audit_cask_instance(cask)
-        summary = audit.summary(include_warnings: output_warnings?)
-        puts summary if summary.present? && output_summary?(audit)
-        warnings += audit.warnings
+        puts audit.summary if audit.summary.present? && output_summary?(audit)
         errors += audit.errors
       end
 
-      { warnings: warnings, errors: errors }
+      { errors: errors }
     end
 
     private
diff --git a/Library/Homebrew/cask/cmd/audit.rb b/Library/Homebrew/cask/cmd/audit.rb
index 0c65d24b22..59d759f8d2 100644
--- a/Library/Homebrew/cask/cmd/audit.rb
+++ b/Library/Homebrew/cask/cmd/audit.rb
@@ -90,7 +90,6 @@ module Cask
           quarantine:            quarantine,
           language:              language,
           any_named_args:        any_named_args,
-          display_failures_only: display_failures_only,
           only:                  only,
           except:                except,
         }.compact
diff --git a/Library/Homebrew/test/cask/audit_spec.rb b/Library/Homebrew/test/cask/audit_spec.rb
index 251aa00217..873fdb954a 100644
--- a/Library/Homebrew/test/cask/audit_spec.rb
+++ b/Library/Homebrew/test/cask/audit_spec.rb
@@ -13,23 +13,14 @@ describe Cask::Audit, :cask do
   end
 
   def passed?(audit)
-    !audit.errors? && !audit.warnings?
+    !audit.errors?
   end
 
   def outcome(audit)
     if passed?(audit)
       "passed"
     else
-      message = ""
-
-      message += "warned with #{audit.warnings.map { |e| e.fetch(:message).inspect }.join(",")}" if audit.warnings?
-
-      if audit.errors?
-        message += " and " if audit.warnings?
-        message += "errored with #{audit.errors.map { |e| e.fetch(:message).inspect }.join(",")}"
-      end
-
-      message
+      "errored with #{audit.errors.map { |e| e.fetch(:message).inspect }.join(",")}"
     end
   end
 
@@ -55,7 +46,7 @@ describe Cask::Audit, :cask do
 
   matcher :warn_with do |message|
     match do |audit|
-      include_msg?(audit.warnings, message)
+      include_msg?(audit.errors, message)
     end
 
     failure_message do |audit|
@@ -118,6 +109,14 @@ describe Cask::Audit, :cask do
   describe "#result" do
     subject { audit.result }
 
+    context "when there are no errors and `--strict` is not passed so we should not show anything" do
+      before do
+        audit.add_warning "eh"
+      end
+
+      it { is_expected.not_to match(/failed/) }
+    end
+
     context "when there are errors" do
       before do
         audit.add_error "bad"
@@ -126,14 +125,6 @@ describe Cask::Audit, :cask do
       it { is_expected.to match(/failed/) }
     end
 
-    context "when there are warnings" do
-      before do
-        audit.add_warning "eh"
-      end
-
-      it { is_expected.to match(/warning/) }
-    end
-
     context "when there are errors and warnings" do
       before do
         audit.add_error "bad"
@@ -143,8 +134,33 @@ describe Cask::Audit, :cask do
       it { is_expected.to match(/failed/) }
     end
 
-    context "when there are no errors or warnings" do
-      it { is_expected.to match(/passed/) }
+    context "when there are errors and warnings and `--strict` is passed" do
+      let(:strict) { true }
+
+      before do
+        audit.add_error "very bad"
+        audit.add_warning "a little bit bad"
+      end
+
+      it { is_expected.to match(/failed/) }
+    end
+
+    context "when there are warnings and `--strict` is not passed" do
+      before do
+        audit.add_warning "a little bit bad"
+      end
+
+      it { is_expected.not_to match(/failed/) }
+    end
+
+    context "when there are warnings and `--strict` is passed" do
+      let(:strict) { true }
+
+      before do
+        audit.add_warning "a little bit bad"
+      end
+
+      it { is_expected.to match(/failed/) }
     end
   end
 
@@ -984,9 +1000,20 @@ describe Cask::Audit, :cask do
       context "when cask token conflicts with a core formula" do
         let(:formula_names) { %w[with-binary other-formula] }
 
-        it "warns about duplicates" do
-          expect(audit).to receive(:core_formula_names).and_return(formula_names)
-          expect(run).to warn_with(/possible duplicate/)
+        context "when `--strict` is passed" do
+          let(:strict) { true }
+
+          it "warns about duplicates" do
+            expect(audit).to receive(:core_formula_names).and_return(formula_names)
+            expect(run).to warn_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/)
+          end
         end
       end
 
@@ -1058,8 +1085,8 @@ describe Cask::Audit, :cask do
       context "when `new_cask` is false" do
         let(:new_cask) { false }
 
-        it "warns" do
-          expect(run).to warn_with(/should have a description/)
+        it "does not warn" do
+          expect(run).not_to warn_with(/should have a description/)
         end
       end
 
diff --git a/Library/Homebrew/test/cask/quarantine_spec.rb b/Library/Homebrew/test/cask/quarantine_spec.rb
index f14d4bd70b..b5c49772da 100644
--- a/Library/Homebrew/test/cask/quarantine_spec.rb
+++ b/Library/Homebrew/test/cask/quarantine_spec.rb
@@ -39,9 +39,7 @@ describe Cask::Quarantine, :cask do
     it "quarantines Cask audits" do
       expect do
         Cask::Cmd::Audit.run("local-transmission", "--download")
-      end.to not_raise_error
-        .and output(/audit for local-transmission: passed/).to_stdout
-                                                           .and not_to_output.to_stderr
+      end.to not_raise_error.and not_to_output.to_stderr
 
       local_transmission = Cask::CaskLoader.load(cask_path("local-transmission"))
       cached_location = Cask::Download.new(local_transmission).fetch
@@ -156,9 +154,7 @@ describe Cask::Quarantine, :cask do
     it "does not quarantine Cask audits" do
       expect do
         Cask::Cmd::Audit.run("local-transmission", "--download", "--no-quarantine")
-      end.to not_raise_error
-        .and output(/audit for local-transmission: passed/).to_stdout
-                                                           .and not_to_output.to_stderr
+      end.to not_raise_error.and not_to_output.to_stderr
 
       local_transmission = Cask::CaskLoader.load(cask_path("local-transmission"))
       cached_location = Cask::Download.new(local_transmission).fetch