From b99e8626e28ef3f3941d7bf9b755fbddcdbcd8b9 Mon Sep 17 00:00:00 2001 From: Sam Ford <1584702+samford@users.noreply.github.com> Date: Fri, 7 Aug 2020 17:25:08 -0400 Subject: [PATCH 1/3] Raise TypeError in Livecheck DSL for invalid arg We previously added `raise TypeError` logic to the `Livecheck` DSL's `#strategy` method. This updates the existing methods to follow suit and modifies the tests accordingly. --- Library/Homebrew/livecheck.rb | 26 ++++++++++++++++++------- Library/Homebrew/test/livecheck_spec.rb | 17 ++++++++++++++++ 2 files changed, 36 insertions(+), 7 deletions(-) diff --git a/Library/Homebrew/livecheck.rb b/Library/Homebrew/livecheck.rb index bb1458f2a10e8..237118345bfab 100644 --- a/Library/Homebrew/livecheck.rb +++ b/Library/Homebrew/livecheck.rb @@ -22,9 +22,14 @@ def initialize(formula) # Sets the regex instance variable to the argument given, returns the # regex instance variable when no argument is given. 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 @@ -32,8 +37,13 @@ def regex(pattern = nil) # its value is assigned to the skip_msg instance variable, else nil is # assigned. 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? @@ -60,15 +70,17 @@ def strategy(symbol = nil) # Sets the url instance variable to the argument given, returns the url # instance variable when no argument is given. 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 diff --git a/Library/Homebrew/test/livecheck_spec.rb b/Library/Homebrew/test/livecheck_spec.rb index fdeb9627a7637..3b1ed99960cc2 100644 --- a/Library/Homebrew/test/livecheck_spec.rb +++ b/Library/Homebrew/test/livecheck_spec.rb @@ -20,6 +20,12 @@ 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 @@ -34,6 +40,12 @@ 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 @@ -69,6 +81,11 @@ it "returns the URL if set" do livecheckable.url("foo") expect(livecheckable.url).to eq("foo") + + 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 From 47d07b2f1baaa2a999c64b768d66ba2f7a80b892 Mon Sep 17 00:00:00 2001 From: Sam Ford <1584702+samford@users.noreply.github.com> Date: Fri, 7 Aug 2020 17:26:55 -0400 Subject: [PATCH 2/3] Improve existing tests for Livecheck DSL --- Library/Homebrew/test/livecheck_spec.rb | 35 ++++++++++++++++++------- 1 file changed, 26 insertions(+), 9 deletions(-) diff --git a/Library/Homebrew/test/livecheck_spec.rb b/Library/Homebrew/test/livecheck_spec.rb index 3b1ed99960cc2..e5ed4a57c958b 100644 --- a/Library/Homebrew/test/livecheck_spec.rb +++ b/Library/Homebrew/test/livecheck_spec.rb @@ -4,19 +4,25 @@ 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 @@ -29,14 +35,14 @@ 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 @@ -49,8 +55,9 @@ 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 @@ -74,7 +81,7 @@ end describe "#url" do - it "returns nil if unset" do + it "returns nil if not set" do expect(livecheckable.url).to be nil end @@ -82,6 +89,16 @@ 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/) From 05b3518b0cb3dac595531014326858016a622ae2 Mon Sep 17 00:00:00 2001 From: Sam Ford <1584702+samford@users.noreply.github.com> Date: Fri, 7 Aug 2020 17:27:31 -0400 Subject: [PATCH 3/3] Improve documentation comments for Livecheck DSL --- Library/Homebrew/livecheck.rb | 50 ++++++++++++++++++++++------------- 1 file changed, 31 insertions(+), 19 deletions(-) diff --git a/Library/Homebrew/livecheck.rb b/Library/Homebrew/livecheck.rb index 237118345bfab..32386389bc655 100644 --- a/Library/Homebrew/livecheck.rb +++ b/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,8 +21,10 @@ def initialize(formula) @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) case pattern when nil @@ -32,10 +36,12 @@ def regex(pattern = nil) 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 @@ -46,16 +52,17 @@ def skip(skip_msg = nil) @skip = true 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 @@ -67,8 +74,12 @@ def strategy(symbol = nil) 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) @url = case val when nil @@ -84,7 +95,8 @@ def url(val = nil) 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,