Merge pull request #8253 from samford/livecheck-dsl-add-raise-update-docs-comments

Raise TypeError in Livecheck DSL methods, expand tests, improve documentation comments

Library/Homebrew/livecheck.rb

@@ -1,13 +1,15 @@
 # frozen_string_literal: true
 
-# Livecheck can be used to check for newer versions of the software.
-# The livecheck DSL specified in the formula is evaluated the methods
-# of this class, which set the instance variables accordingly. The
-# information is used by brew livecheck when checking for newer versions
-# of the software.
+# The `Livecheck` class implements the DSL methods used in a formula's
+# `livecheck` block and stores related instance variables. Most of these methods
+# also return the related instance variable when no argument is provided.
+#
+# This information is used by the `brew livecheck` command to control its
+# behavior.
 class Livecheck
-  # The reason for skipping livecheck for the formula.
-  # e.g. `Not maintained`
+  # A very brief description of why the formula is skipped (e.g., `No longer
+  # developed or maintained`).
+  # @return [String, nil]
   attr_reader :skip_msg
 
   def initialize(formula)
@@ -19,33 +21,48 @@ class Livecheck
     @url = nil
   end
 
-  # Sets the regex instance variable to the argument given, returns the
-  # regex instance variable when no argument is given.
+  # Sets the `@regex` instance variable to the provided `Regexp` or returns the
+  # `@regex` instance variable when no argument is provided.
+  # @param pattern [Regexp] regex to use for matching versions in content
+  # @return [Regexp, nil]
   def regex(pattern = nil)
-    return @regex if pattern.nil?
-
-    @regex = pattern
+    case pattern
+    when nil
+      @regex
+    when Regexp
+      @regex = pattern
+    else
+      raise TypeError, "Livecheck#regex expects a Regexp"
+    end
   end
 
-  # Sets the skip instance variable to true, indicating that livecheck
-  # must be skipped for the formula. If an argument is given and present,
-  # its value is assigned to the skip_msg instance variable, else nil is
-  # assigned.
+  # Sets the `@skip` instance variable to `true` and sets the `@skip_msg`
+  # instance variable if a `String` is provided. `@skip` is used to indicate
+  # that the formula should be skipped and the `skip_msg` very briefly describes
+  # why the formula is skipped (e.g., `No longer developed or maintained`).
+  # @param skip_msg [String] string describing why the formula is skipped
+  # @return [Boolean]
   def skip(skip_msg = nil)
+    if skip_msg.is_a?(String)
+      @skip_msg = skip_msg
+    elsif skip_msg.present?
+      raise TypeError, "Livecheck#skip expects a String"
+    end
+
     @skip = true
-    @skip_msg = skip_msg.presence
   end
 
-  # Should livecheck be skipped for the formula?
+  # Should `livecheck` skip this formula?
   def skip?
     @skip
   end
 
-  # Sets the strategy instance variable to the provided symbol or returns the
-  # strategy instance variable when no argument is provided. The strategy
+  # Sets the `@strategy` instance variable to the provided `Symbol` or returns
+  # the `@strategy` instance variable when no argument is provided. The strategy
   # symbols use snake case (e.g., `:page_match`) and correspond to the strategy
   # file name.
   # @param symbol [Symbol] symbol for the desired strategy
+  # @return [Symbol, nil]
   def strategy(symbol = nil)
     case symbol
     when nil
@@ -57,22 +74,29 @@ class Livecheck
     end
   end
 
-  # Sets the url instance variable to the argument given, returns the url
-  # instance variable when no argument is given.
+  # Sets the `@url` instance variable to the provided argument or returns the
+  # `@url` instance variable when no argument is provided. The argument can be
+  # a `String` (a URL) or a supported `Symbol` corresponding to a URL in the
+  # formula (e.g., `:stable`, `:homepage`, or `:head`).
+  # @param val [String, Symbol] URL to check for version information
+  # @return [String, nil]
   def url(val = nil)
-    return @url if val.nil?
-
     @url = case val
+    when nil
+      return @url
     when :head, :stable, :devel
       @formula.send(val).url
     when :homepage
       @formula.homepage
-    else
+    when String
       val
+    else
+      raise TypeError, "Livecheck#url expects a String or valid Symbol"
     end
   end
 
-  # Returns a Hash of all instance variable values.
+  # Returns a `Hash` of all instance variable values.
+  # @return [Hash]
   def to_hash
     {
       "regex"    => @regex,

Library/Homebrew/test/livecheck_spec.rb

@@ -4,41 +4,60 @@ require "formula"
 require "livecheck"
 
 describe Livecheck do
+  HOMEPAGE_URL = "https://example.com/"
+  STABLE_URL = "https://example.com/example-1.2.3.tar.gz"
+  HEAD_URL = "https://example.com/example.git"
+
   let(:f) do
     formula do
-      url "https://brew.sh/test-0.1.tbz"
+      homepage HOMEPAGE_URL
+      url STABLE_URL
+      head HEAD_URL
     end
   end
   let(:livecheckable) { described_class.new(f) }
 
   describe "#regex" do
-    it "returns nil if unset" do
+    it "returns nil if not set" do
       expect(livecheckable.regex).to be nil
     end
 
-    it "returns the Regex if set" do
+    it "returns the Regexp if set" do
       livecheckable.regex(/foo/)
       expect(livecheckable.regex).to eq(/foo/)
     end
+
+    it "raises a TypeError if the argument isn't a Regexp" do
+      expect {
+        livecheckable.regex("foo")
+      }.to raise_error(TypeError, "Livecheck#regex expects a Regexp")
+    end
   end
 
   describe "#skip" do
-    it "sets the instance variable skip to true and skip_msg to nil when the argument is not present" do
-      livecheckable.skip
+    it "sets @skip to true when no argument is provided" do
+      expect(livecheckable.skip).to be true
       expect(livecheckable.instance_variable_get(:@skip)).to be true
       expect(livecheckable.instance_variable_get(:@skip_msg)).to be nil
     end
 
-    it "sets the instance variable skip to true and skip_msg to the argument when present" do
-      livecheckable.skip("foo")
+    it "sets @skip to true and @skip_msg to the provided String" do
+      expect(livecheckable.skip("foo")).to be true
       expect(livecheckable.instance_variable_get(:@skip)).to be true
       expect(livecheckable.instance_variable_get(:@skip_msg)).to eq("foo")
     end
+
+    it "raises a TypeError if the argument isn't a String" do
+      expect {
+        livecheckable.skip(/foo/)
+      }.to raise_error(TypeError, "Livecheck#skip expects a String")
+    end
   end
 
   describe "#skip?" do
-    it "returns the value of the instance variable skip" do
+    it "returns the value of @skip" do
       expect(livecheckable.skip?).to be false
+
       livecheckable.skip
       expect(livecheckable.skip?).to be true
     end
@@ -62,13 +81,28 @@ describe Livecheck do
   end
 
   describe "#url" do
-    it "returns nil if unset" do
+    it "returns nil if not set" do
       expect(livecheckable.url).to be nil
     end
 
     it "returns the URL if set" do
       livecheckable.url("foo")
       expect(livecheckable.url).to eq("foo")
+
+      livecheckable.url(:homepage)
+      expect(livecheckable.url).to eq(HOMEPAGE_URL)
+
+      livecheckable.url(:stable)
+      expect(livecheckable.url).to eq(STABLE_URL)
+
+      livecheckable.url(:head)
+      expect(livecheckable.url).to eq(HEAD_URL)
+    end
+
+    it "raises a TypeError if the argument isn't a String or Symbol" do
+      expect {
+        livecheckable.url(/foo/)
+      }.to raise_error(TypeError, "Livecheck#url expects a String or valid Symbol")
     end
   end