bump: print skip status, message(s), and errors

`brew livecheck` surfaces the skip reason or message in its output
(e.g., "deprecated", "skipped - Legacy version") but `brew bump`
effectively ignores the `:status` and `:messages` values from
`Livecheck::SkipConditions` and only prints "skipped" instead.
Sometimes the reason can be inferred from parenthetical annotations
(e.g., "(deprecated)" after the current version) but explicit output
like "skipped - deprecated" is easier to read when skimming. Omitting
a skip message from a `livecheck` block is only done to produce a
predictable "skipped" string that can be used for comparison but we
can handle that in a more robust way.

A related issue we're now facing is that deprecated packages are
producing an "unable to get versions" message rather than "skipped".
This adds noise and makes it more challenging to identify real
livecheck failures in bump output.

This addresses these issues by modifying bump to print the skip
`:status` and `:messages` values in the output. The way I've handled
this, bump will also print error messages. In the process, this adds
a `message?` method to check whether a value is a message string,
replacing existing conditions that insufficiently check whether a
value is a message using strict comparisons like `x != "skipped"` or
`x != "unable to get versions"` and ignoring the possibility of error
strings.

This is still an imperfect way of identifying message strings but it
should be an improvement over the status quo. In the future, it may be
better to store message strings as something other than a
`Cask::DSL::Version` object, so we can distinguish messages from
versions without targeting a specific string pattern.

Author: samford

Library/Homebrew/dev-cmd/bump.rb

@@ -278,8 +278,12 @@ def livecheck_result(formula_or_cask)
         )
 
         if skip_info.present?
-          return "#{skip_info[:status]}" \
-                 "#{" - #{skip_info[:messages].join("; ")}" if skip_info[:messages].present?}"
+          skip_status = skip_info[:status]
+          skip_messages = skip_info[:messages]
+          skip_message = skip_messages.join("; ") if skip_messages.present?
+          return "error: #{skip_message}" if skip_status == "error" && skip_message
+
+          return "skipped - #{skip_message || skip_status}"
         end
 
         version_info = Livecheck.latest_version(
@@ -369,10 +373,9 @@ def retrieve_versions_by_arch(formula_or_cask:, repositories:, name:)
             new_version_value = if (livecheck_latest_is_a_version &&
                                     Livecheck::LivecheckVersion.create(formula_or_cask, livecheck_latest) >=
                                     Livecheck::LivecheckVersion.create(formula_or_cask, current_version_value)) ||
-                                   current_version_value == "latest"
+                                   current_version_value == "latest" ||
+                                   message?(livecheck_latest)
               livecheck_latest
-            elsif livecheck_latest.is_a?(String) && livecheck_latest.start_with?("skipped")
-              "skipped"
             elsif repology_latest_is_a_version &&
                   !formula_or_cask_has_livecheck &&
                   repology_latest > current_version_value &&
@@ -433,21 +436,18 @@ def retrieve_versions_by_arch(formula_or_cask:, repositories:, name:)
            !newer_than_upstream.all? { |_k, v| v == true }
           pull_request_version = nil
           if (new_version_arm = new_version.arm) &&
-             (new_version_arm != "unable to get versions") &&
-             (new_version_arm != "skipped") &&
+             !message?(new_version_arm) &&
              (new_version_arm != current_version.arm)
             # We use the ARM version for the pull request version even if there
             # are multiple arch versions to be consistent with the behavior of
             # bump-cask-pr.
             pull_request_version = new_version_arm.to_s
           elsif (new_version_intel = new_version.intel) &&
-                (new_version_intel != "unable to get versions") &&
-                (new_version_intel != "skipped") &&
+                !message?(new_version_intel) &&
                 (new_version_intel != current_version.intel)
             pull_request_version = new_version_intel.to_s
           elsif (new_version_general = new_version.general) &&
-                (new_version_general != "unable to get versions") &&
-                (new_version_general != "skipped") &&
+                !message?(new_version_general) &&
                 (new_version_general != current_version.general)
             pull_request_version = new_version_general.to_s
           end
@@ -631,8 +631,7 @@ def retrieve_and_display_info_and_open_pr(formula_or_cask, name, repositories, a
         end
 
         if !args.no_pull_requests? &&
-           (new_version.general != "unable to get versions") &&
-           (new_version.general != "skipped") &&
+           !message?(new_version.general) &&
            !versions_equal &&
            !all_newer_than_upstream
           if duplicate_pull_requests
@@ -652,8 +651,7 @@ def retrieve_and_display_info_and_open_pr(formula_or_cask, name, repositories, a
         end
 
         if !args.open_pr? ||
-           (new_version.general == "unable to get versions") ||
-           (new_version.general == "skipped") ||
+           message?(new_version.general) ||
            all_newer_than_upstream
           return
         end
@@ -678,15 +676,13 @@ def retrieve_and_display_info_and_open_pr(formula_or_cask, name, repositories, a
         version_args = []
         if multiple_versions[:current] && multiple_versions[:new]
           if (new_version_arm = new_version.arm) &&
-             new_version_arm != "unable to get versions" &&
-             new_version_arm != "skipped" &&
+             !message?(new_version_arm) &&
              current_version.arm &&
              new_version_arm > current_version.arm
             version_args << "--version-arm=#{new_version_arm}"
           end
           if (new_version_intel = new_version.intel) &&
-             new_version_intel != "unable to get versions" &&
-             new_version_intel != "skipped" &&
+             !message?(new_version_intel) &&
              current_version.intel &&
              new_version_intel > current_version.intel
             version_args << "--version-intel=#{new_version_intel}"
@@ -745,8 +741,7 @@ def compare_versions(current_version, new_version, formula_or_cask)
           end
 
           new_version_value = new_version.send(type)
-          if new_version_value.is_a?(String) &&
-             new_version_value.match?(LIVECHECK_MESSAGE_REGEX)
+          if message?(new_version_value)
             # Store a string, so we can easily tell when a value is a message
             # rather than a version
             new_versions[type] = new_version_value.to_s
@@ -803,6 +798,13 @@ def compare_versions(current_version, new_version, formula_or_cask)
         { multiple_versions:, newer_than_upstream: }
       end
 
+      sig { params(value: T.nilable(T.any(Version, Cask::DSL::Version, String))).returns(T::Boolean) }
+      def message?(value)
+        return false if !value.is_a?(Cask::DSL::Version) && !value.is_a?(String)
+
+        value.match?(LIVECHECK_MESSAGE_REGEX)
+      end
+
       sig {
         params(
           formula:     Formula,

Library/Homebrew/test/dev-cmd/bump_spec.rb

@@ -152,4 +152,35 @@
       })
     end
   end
+
+  describe "::message?" do
+    let(:version) { Version.new("1.2.3") }
+    let(:cask_version) { Cask::DSL::Version.new("1.2.3,4") }
+    let(:message_strings) do
+      [
+        "error: message",
+        "skipped",
+        "skipped - deprecated",
+        "unable to get versions",
+        "unable to get throttled versions",
+      ]
+    end
+
+    it "returns false when value is not a `Cask::DSL::Version` or string" do
+      expect(bump.send(:message?, version)).to be(false)
+      expect(bump.send(:message?, nil)).to be(false)
+    end
+
+    it "returns false when `Cask::DSL::Version` or string is not a message" do
+      expect(bump.send(:message?, cask_version)).to be(false)
+      expect(bump.send(:message?, "Not a message string")).to be(false)
+    end
+
+    it "returns true when `Cask::DSL::Version` or string is a message" do
+      message_strings.each do |message_string|
+        expect(bump.send(:message?, Cask::DSL::Version.new(message_string))).to be(true)
+        expect(bump.send(:message?, message_string)).to be(true)
+      end
+    end
+  end
 end