From 8afa354c35c6b68c4f9f07e936a2479041952d56 Mon Sep 17 00:00:00 2001 From: Sam Ford <1584702+samford@users.noreply.github.com> Date: Thu, 13 Feb 2025 21:32:51 -0500 Subject: [PATCH] Livecheck::Options: Rework as T::Struct As suggested, this reworks `Options` to subclass `T::Struct`, which simplifies the implementation and makes it easier to maintain. One noteworthy difference in switching to `T::Struct` is that `#serialize` omits `nil` values but I don't _think_ this should be a problem for us. In terms of changes, I modified `#merge` to remove a now-unnecessary `compact` call and updated related tests. Co-authored-by: Douglas Eichelberger <697964+dduugg@users.noreply.github.com> --- Library/Homebrew/livecheck.rb | 10 +--- Library/Homebrew/livecheck/options.rb | 59 ++++--------------- .../Homebrew/test/livecheck/options_spec.rb | 24 ++++---- 3 files changed, 29 insertions(+), 64 deletions(-) diff --git a/Library/Homebrew/livecheck.rb b/Library/Homebrew/livecheck.rb index 98a48ce49d..10e7a91f99 100644 --- a/Library/Homebrew/livecheck.rb +++ b/Library/Homebrew/livecheck.rb @@ -179,13 +179,9 @@ class Livecheck def url(url = T.unsafe(nil), homebrew_curl: nil, post_form: nil, post_json: nil) raise ArgumentError, "Only use `post_form` or `post_json`, not both" if post_form && post_json - if homebrew_curl || post_form || post_json - @options = @options.merge({ - homebrew_curl:, - post_form:, - post_json:, - }.compact) - end + @options.homebrew_curl = homebrew_curl unless homebrew_curl.nil? + @options.post_form = post_form unless post_form.nil? + @options.post_json = post_json unless post_json.nil? case url when nil diff --git a/Library/Homebrew/livecheck/options.rb b/Library/Homebrew/livecheck/options.rb index 8e8c421758..e4290b5203 100644 --- a/Library/Homebrew/livecheck/options.rb +++ b/Library/Homebrew/livecheck/options.rb @@ -8,34 +8,15 @@ module Homebrew # # Option values use a `nil` default to indicate that the value has not been # set. - class Options + class Options < T::Struct # Whether to use brewed curl. - sig { returns(T.nilable(T::Boolean)) } - attr_reader :homebrew_curl + prop :homebrew_curl, T.nilable(T::Boolean) # Form data to use when making a `POST` request. - sig { returns(T.nilable(T::Hash[Symbol, String])) } - attr_reader :post_form + prop :post_form, T.nilable(T::Hash[Symbol, String]) # JSON data to use when making a `POST` request. - sig { returns(T.nilable(T::Hash[Symbol, String])) } - attr_reader :post_json - - # @param homebrew_curl whether to use brewed curl - # @param post_form form data to use when making a `POST` request - # @param post_json JSON data to use when making a `POST` request - sig { - params( - homebrew_curl: T.nilable(T::Boolean), - post_form: T.nilable(T::Hash[Symbol, String]), - post_json: T.nilable(T::Hash[Symbol, String]), - ).void - } - def initialize(homebrew_curl: nil, post_form: nil, post_json: nil) - @homebrew_curl = homebrew_curl - @post_form = post_form - @post_json = post_json - end + prop :post_json, T.nilable(T::Hash[Symbol, String]) # Returns a `Hash` of options that are provided as arguments to `url`. sig { returns(T::Hash[Symbol, T.untyped]) } @@ -47,25 +28,13 @@ module Homebrew } end - # Returns a `Hash` of all instance variables, using `Symbol` keys. - sig { returns(T::Hash[Symbol, T.untyped]) } - def to_h - { - homebrew_curl:, - post_form:, - post_json:, - } - end - # Returns a `Hash` of all instance variables, using `String` keys. sig { returns(T::Hash[String, T.untyped]) } - def to_hash - { - "homebrew_curl" => @homebrew_curl, - "post_form" => @post_form, - "post_json" => @post_json, - } - end + def to_hash = serialize + + # Returns a `Hash` of all instance variables, using `Symbol` keys. + sig { returns(T::Hash[Symbol, T.untyped]) } + def to_h = serialize.transform_keys(&:to_sym) # Returns a new object formed by merging `other` values with a copy of # `self`. @@ -78,7 +47,7 @@ module Homebrew return dup if other.empty? this_hash = to_h - other_hash = other.is_a?(Options) ? other.to_h.compact : other + other_hash = other.is_a?(Options) ? other.to_h : other return dup if this_hash == other_hash new_options = this_hash.merge(other_hash) @@ -96,15 +65,11 @@ module Homebrew # Whether the object has only default values. sig { returns(T::Boolean) } - def empty? - @homebrew_curl.nil? && @post_form.nil? && @post_json.nil? - end + def empty? = serialize.empty? # Whether the object has any non-default values. sig { returns(T::Boolean) } - def present? - !@homebrew_curl.nil? || !@post_form.nil? || !@post_json.nil? - end + def present? = !empty? end end end diff --git a/Library/Homebrew/test/livecheck/options_spec.rb b/Library/Homebrew/test/livecheck/options_spec.rb index 38be0f0b9b..d9ced1bff1 100644 --- a/Library/Homebrew/test/livecheck/options_spec.rb +++ b/Library/Homebrew/test/livecheck/options_spec.rb @@ -39,21 +39,19 @@ RSpec.describe Homebrew::Livecheck::Options do describe "#to_h" do it "returns a Hash of all instance variables" do - expect(options.new.to_h).to eq({ - homebrew_curl: nil, - post_form: nil, - post_json: nil, - }) + # `T::Struct.serialize` omits `nil` values + expect(options.new.to_h).to eq({}) + + expect(options.new(**args).to_h).to eq(args) end end describe "#to_hash" do it "returns a Hash of all instance variables, using String keys" do - expect(options.new.to_hash).to eq({ - "homebrew_curl" => nil, - "post_form" => nil, - "post_json" => nil, - }) + # `T::Struct.serialize` omits `nil` values + expect(options.new.to_hash).to eq({}) + + expect(options.new(**args).to_hash).to eq(args.transform_keys(&:to_s)) end end @@ -65,6 +63,8 @@ RSpec.describe Homebrew::Livecheck::Options do .to eq(options.new(**merged_hash)) expect(options.new(**args).merge(args)) .to eq(options.new(**args)) + expect(options.new(**args).merge({})) + .to eq(options.new(**args)) end end @@ -82,6 +82,10 @@ RSpec.describe Homebrew::Livecheck::Options do it "returns false if any instance variables differ" do expect(options.new == options.new(**args)).to be false end + + it "returns false if other object is not the same class" do + expect(options.new == :other).to be false + end end describe "#empty?" do