Merge pull request #11842 from samford/livecheck/refactor-strategy-bl…

…ock-handling

Refactor livecheck strategy block handling

Library/Homebrew/livecheck/livecheck.rb

@@ -56,9 +56,11 @@ def livecheck_strategy_names
 
       # Cache demodulized strategy names, to avoid repeating this work
       @livecheck_strategy_names = {}
-      Strategy.constants.sort.each do |strategy_symbol|
-        strategy = Strategy.const_get(strategy_symbol)
-        @livecheck_strategy_names[strategy] = strategy.name.demodulize
+      Strategy.constants.sort.each do |const_symbol|
+        constant = Strategy.const_get(const_symbol)
+        next unless constant.is_a?(Class)
+
+        @livecheck_strategy_names[constant] = T.must(constant.name).demodulize
       end
       @livecheck_strategy_names.freeze
     end

Library/Homebrew/livecheck/strategy.rb

@@ -14,7 +14,7 @@ module Strategy
 
       module_function
 
-      # Strategy priorities informally range from 1 to 10, where 10 is the
+      # {Strategy} priorities informally range from 1 to 10, where 10 is the
       # highest priority. 5 is the default priority because it's roughly in
       # the middle of this range. Strategies with a priority of 0 (or lower)
       # are ignored.
@@ -32,10 +32,10 @@ module Strategy
       # The `curl` process will sometimes hang indefinitely (despite setting
       # the `--max-time` argument) and it needs to be quit for livecheck to
       # continue. This value is used to set the `timeout` argument on
-      # `Utils::Curl` method calls in `Strategy`.
+      # `Utils::Curl` method calls in {Strategy}.
       CURL_PROCESS_TIMEOUT = CURL_MAX_TIME + 5
 
-      # Baseline `curl` arguments used in `Strategy` methods.
+      # Baseline `curl` arguments used in {Strategy} methods.
       DEFAULT_CURL_ARGS = [
         # Follow redirections to handle mirrors, relocations, etc.
         "--location",
@@ -60,7 +60,7 @@ module Strategy
         "--include",
       ] + DEFAULT_CURL_ARGS).freeze
 
-      # Baseline `curl` options used in `Strategy` methods.
+      # Baseline `curl` options used in {Strategy} methods.
       DEFAULT_CURL_OPTIONS = {
         print_stdout: false,
         print_stderr: false,
@@ -75,52 +75,66 @@ module Strategy
       # In rare cases, this can also be a double newline (`\n\n`).
       HTTP_HEAD_BODY_SEPARATOR = "\r\n\r\n"
 
-      # The `#strategies` method expects `Strategy` constants to be strategies,
-      # so constants we create need to be private for this to work properly.
-      private_constant :DEFAULT_PRIORITY, :CURL_CONNECT_TIMEOUT, :CURL_MAX_TIME,
-                       :CURL_PROCESS_TIMEOUT, :DEFAULT_CURL_ARGS,
-                       :PAGE_HEADERS_CURL_ARGS, :PAGE_CONTENT_CURL_ARGS,
-                       :DEFAULT_CURL_OPTIONS, :HTTP_HEAD_BODY_SEPARATOR
+      # An error message to use when a `strategy` block returns a value of
+      # an inappropriate type.
+      INVALID_BLOCK_RETURN_VALUE_MSG = "Return value of a strategy block must be a string or array of strings."
 
       # Creates and/or returns a `@strategies` `Hash`, which maps a snake
       # case strategy name symbol (e.g. `:page_match`) to the associated
-      # {Strategy}.
+      # strategy.
       #
       # At present, this should only be called after tap strategies have been
       # loaded, otherwise livecheck won't be able to use them.
       # @return [Hash]
+      sig { returns(T::Hash[Symbol, T.untyped]) }
       def strategies
         return @strategies if defined? @strategies
 
         @strategies = {}
-        constants.sort.each do |strategy_symbol|
-          key = strategy_symbol.to_s.underscore.to_sym
-          strategy = const_get(strategy_symbol)
-          @strategies[key] = strategy
+        Strategy.constants.sort.each do |const_symbol|
+          constant = Strategy.const_get(const_symbol)
+          next unless constant.is_a?(Class)
+
+          key = const_symbol.to_s.underscore.to_sym
+          @strategies[key] = constant
         end
         @strategies
       end
       private_class_method :strategies
 
-      # Returns the {Strategy} that corresponds to the provided `Symbol` (or
-      # `nil` if there is no matching {Strategy}).
+      # Returns the strategy that corresponds to the provided `Symbol` (or
+      # `nil` if there is no matching strategy).
       #
-      # @param symbol [Symbol] the strategy name in snake case as a `Symbol`
-      #   (e.g. `:page_match`)
-      # @return [Strategy, nil]
+      # @param symbol [Symbol, nil] the strategy name in snake case as a
+      #   `Symbol` (e.g. `:page_match`)
+      # @return [Class, nil]
+      sig { params(symbol: T.nilable(Symbol)).returns(T.nilable(T.untyped)) }
       def from_symbol(symbol)
-        strategies[symbol]
+        strategies[symbol] if symbol.present?
       end
 
       # Returns an array of strategies that apply to the provided URL.
       #
       # @param url [String] the URL to check for matching strategies
-      # @param livecheck_strategy [Symbol] a {Strategy} symbol from the
+      # @param livecheck_strategy [Symbol] a strategy symbol from the
+      #   `livecheck` block
+      # @param url_provided [Boolean] whether a url is provided in the
       #   `livecheck` block
       # @param regex_provided [Boolean] whether a regex is provided in the
       #   `livecheck` block
+      # @param block_provided [Boolean] whether a `strategy` block is provided
+      #   in the `livecheck` block
       # @return [Array]
-      def from_url(url, livecheck_strategy: nil, url_provided: nil, regex_provided: nil, block_provided: nil)
+      sig {
+        params(
+          url:                String,
+          livecheck_strategy: T.nilable(Symbol),
+          url_provided:       T::Boolean,
+          regex_provided:     T::Boolean,
+          block_provided:     T::Boolean,
+        ).returns(T::Array[T.untyped])
+      }
+      def from_url(url, livecheck_strategy: nil, url_provided: false, regex_provided: false, block_provided: false)
         usable_strategies = strategies.values.select do |strategy|
           if strategy == PageMatch
             # Only treat the `PageMatch` strategy as usable if a regex is
@@ -144,6 +158,13 @@ def from_url(url, livecheck_strategy: nil, url_provided: nil, regex_provided: ni
         end
       end
 
+      # Collects HTTP response headers, starting with the provided URL.
+      # Redirections will be followed and all the response headers are
+      # collected into an array of hashes.
+      #
+      # @param url [String] the URL to fetch
+      # @return [Array]
+      sig { params(url: String).returns(T::Array[T::Hash[String, String]]) }
       def self.page_headers(url)
         headers = []
 
@@ -223,6 +244,25 @@ def self.page_content(url)
           messages: [error_msg.presence || "cURL failed without an error"],
         }
       end
+
+      # Handles the return value from a `strategy` block in a `livecheck`
+      # block.
+      #
+      # @param value [] the return value from a `strategy` block
+      # @return [Array]
+      sig { params(value: T.untyped).returns(T::Array[String]) }
+      def self.handle_block_return(value)
+        case value
+        when String
+          [value]
+        when Array
+          value.compact.uniq
+        when nil
+          []
+        else
+          raise TypeError, INVALID_BLOCK_RETURN_VALUE_MSG
+        end
+      end
     end
   end
 end

Library/Homebrew/livecheck/strategy/apache.rb

@@ -37,6 +37,7 @@ class Apache
         #
         # @param url [String] the URL to match against
         # @return [Boolean]
+        sig { params(url: String).returns(T::Boolean) }
         def self.match?(url)
           URL_MATCH_REGEX.match?(url)
         end

Library/Homebrew/livecheck/strategy/bitbucket.rb

@@ -44,6 +44,7 @@ class Bitbucket
         #
         # @param url [String] the URL to match against
         # @return [Boolean]
+        sig { params(url: String).returns(T::Boolean) }
         def self.match?(url)
           URL_MATCH_REGEX.match?(url)
         end

Library/Homebrew/livecheck/strategy/cpan.rb

@@ -35,6 +35,7 @@ class Cpan
         #
         # @param url [String] the URL to match against
         # @return [Boolean]
+        sig { params(url: String).returns(T::Boolean) }
         def self.match?(url)
           URL_MATCH_REGEX.match?(url)
         end

Library/Homebrew/livecheck/strategy/electron_builder.rb

@@ -7,15 +7,17 @@ module Strategy
       # The {ElectronBuilder} strategy fetches content at a URL and parses
       # it as an electron-builder appcast in YAML format.
       #
+      # This strategy is not applied automatically and it's necessary to use
+      # `strategy :electron_builder` in a `livecheck` block to apply it.
+      #
       # @api private
       class ElectronBuilder
         extend T::Sig
 
         NICE_NAME = "electron-builder"
 
         # A priority of zero causes livecheck to skip the strategy. We do this
-        # for {ElectronBuilder} so we can selectively apply the strategy using
-        # `strategy :electron_builder` in a `livecheck` block.
+        # for {ElectronBuilder} so we can selectively apply it when appropriate.
         PRIORITY = 0
 
         # The `Regexp` used to determine if the strategy applies to the URL.
@@ -30,40 +32,34 @@ def self.match?(url)
           URL_MATCH_REGEX.match?(url)
         end
 
-        # Extract version information from page content.
+        # Parses YAML text and identifies versions in it.
         #
-        # @param content [String] the content to check
-        # @return [String]
+        # @param content [String] the YAML text to parse and check
+        # @return [Array]
         sig {
           params(
             content: String,
-            block:   T.nilable(T.proc.params(arg0: T::Hash[String, T.untyped]).returns(T.nilable(String))),
-          ).returns(T.nilable(String))
+            block:   T.nilable(
+              T.proc.params(arg0: T::Hash[String, T.untyped]).returns(T.any(String, T::Array[String], NilClass)),
+            ),
+          ).returns(T::Array[String])
         }
-        def self.version_from_content(content, &block)
+        def self.versions_from_content(content, &block)
           require "yaml"
 
           yaml = YAML.safe_load(content)
-          return if yaml.blank?
+          return [] if yaml.blank?
 
-          if block
-            case (value = block.call(yaml))
-            when String
-              return value
-            when nil
-              return
-            else
-              raise TypeError, "Return value of `strategy :electron_builder` block must be a string."
-            end
-          end
+          return Strategy.handle_block_return(block.call(yaml)) if block
 
-          yaml["version"]
+          version = yaml["version"]
+          version.present? ? [version] : []
         end
 
-        # Checks the content at the URL for new versions.
+        # Checks the YAML content at the URL for new versions.
         #
         # @param url [String] the URL of the content to check
-        # @param regex [Regexp] a regex used for matching versions in content
+        # @param regex [Regexp, nil] a regex used for matching versions
         # @return [Hash]
         sig {
           params(
@@ -81,8 +77,9 @@ def self.find_versions(url, regex, cask: nil, &block)
           match_data.merge!(Strategy.page_content(url))
           content = match_data.delete(:content)
 
-          version = version_from_content(content, &block)
-          match_data[:matches][version] = Version.new(version) if version
+          versions_from_content(content, &block).each do |version_text|
+            match_data[:matches][version_text] = Version.new(version_text)
+          end
 
           match_data
         end

Library/Homebrew/livecheck/strategy/extract_plist.rb

@@ -3,31 +3,35 @@
 
 require "bundle_version"
 require "unversioned_cask_checker"
-require_relative "page_match"
 
 module Homebrew
   module Livecheck
     module Strategy
-      # The {ExtractPlist} strategy downloads the file at a URL and
-      # extracts versions from contained `.plist` files.
+      # The {ExtractPlist} strategy downloads the file at a URL and extracts
+      # versions from contained `.plist` files using {UnversionedCaskChecker}.
+      #
+      # In practice, this strategy operates by downloading very large files,
+      # so it's both slow and data-intensive. As such, the {ExtractPlist}
+      # strategy should only be used as an absolute last resort.
+      #
+      # This strategy is not applied automatically and it's necessary to use
+      # `strategy :extract_plist` in a `livecheck` block to apply it.
       #
       # @api private
       class ExtractPlist
         extend T::Sig
 
-        # A priority of zero causes livecheck to skip the strategy. We only
-        # apply {ExtractPlist} using `strategy :extract_plist` in a `livecheck` block,
-        # as we can't automatically determine when this can be successfully
-        # applied to a URL without fetching the content.
+        # A priority of zero causes livecheck to skip the strategy. We do this
+        # for {ExtractPlist} so we can selectively apply it when appropriate.
         PRIORITY = 0
 
         # The `Regexp` used to determine if the strategy applies to the URL.
         URL_MATCH_REGEX = %r{^https?://}i.freeze
 
         # Whether the strategy can be applied to the provided URL.
-        # The strategy will technically match any HTTP URL but is
-        # only usable with a `livecheck` block containing a regex
-        # or block.
+        #
+        # @param url [String] the URL to match against
+        # @return [Boolean]
         sig { params(url: String).returns(T::Boolean) }
         def self.match?(url)
           URL_MATCH_REGEX.match?(url)
@@ -50,13 +54,37 @@ def self.match?(url)
           delegate short_version: :bundle_version
         end
 
-        # Checks the content at the URL for new versions.
+        # Identify versions from `Item`s produced using
+        # {UnversionedCaskChecker} version information.
+        #
+        # @param items [Hash] a hash of `Item`s containing version information
+        # @return [Array]
+        sig {
+          params(
+            items: T::Hash[String, Item],
+            block: T.nilable(
+              T.proc.params(arg0: T::Hash[String, Item]).returns(T.any(String, T::Array[String], NilClass)),
+            ),
+          ).returns(T::Array[String])
+        }
+        def self.versions_from_items(items, &block)
+          return Strategy.handle_block_return(block.call(items)) if block
+
+          items.map do |_key, item|
+            item.bundle_version.nice_version
+          end.compact.uniq
+        end
+
+        # Uses {UnversionedCaskChecker} on the provided cask to identify
+        # versions from `plist` files.
         sig {
           params(
             url:   String,
             regex: T.nilable(Regexp),
             cask:  Cask::Cask,
-            block: T.nilable(T.proc.params(arg0: T::Hash[String, Item]).returns(T.nilable(String))),
+            block: T.nilable(
+              T.proc.params(arg0: T::Hash[String, Item]).returns(T.any(String, T::Array[String], NilClass)),
+            ),
           ).returns(T::Hash[Symbol, T.untyped])
         }
         def self.find_versions(url, regex, cask:, &block)
@@ -66,22 +94,10 @@ def self.find_versions(url, regex, cask:, &block)
           match_data = { matches: {}, regex: regex, url: url }
 
           unversioned_cask_checker = UnversionedCaskChecker.new(cask)
-          versions = unversioned_cask_checker.all_versions.transform_values { |v| Item.new(bundle_version: v) }
-
-          if block
-            case (value = block.call(versions))
-            when String
-              match_data[:matches][value] = Version.new(value)
-            when nil
-              return match_data
-            else
-              raise TypeError, "Return value of `strategy :extract_plist` block must be a string."
-            end
-          elsif versions.any?
-            versions.each_value do |item|
-              version = item.bundle_version.nice_version
-              match_data[:matches][version] = Version.new(version)
-            end
+          items = unversioned_cask_checker.all_versions.transform_values { |v| Item.new(bundle_version: v) }
+
+          versions_from_items(items, &block).each do |version_text|
+            match_data[:matches][version_text] = Version.new(version_text)
           end
 
           match_data