From 9bfe423a5aeaa4ee8c726458d1d5cbdb6e74e802 Mon Sep 17 00:00:00 2001 From: Sam Ford <1584702+samford@users.noreply.github.com> Date: Fri, 17 Nov 2023 18:06:20 -0500 Subject: [PATCH] Xml: Add #element_text method This refactors verbose code in the `Sparkle` strategy where we access element text into a reusable `Xml#element_text` method, replacing chained calls like `item.elements["title"]&.text&.strip&.presence` with `Xml.element_text(item, "title")`. `#element_text` is only used to retrieve the text of a child element in the `Sparkle` strategy but it can also retrieve the text from the provided element if the `child_path` argument is omitted (i.e., `Xml.element_text(item)`). This will allow us to also avoid similar calls like `item.text.strip.presence` in the future. --- .../Homebrew/livecheck/strategy/sparkle.rb | 16 ++++---- Library/Homebrew/livecheck/strategy/xml.rb | 24 +++++++++++ .../test/livecheck/strategy/xml_spec.rb | 41 +++++++++++++++++++ 3 files changed, 73 insertions(+), 8 deletions(-) diff --git a/Library/Homebrew/livecheck/strategy/sparkle.rb b/Library/Homebrew/livecheck/strategy/sparkle.rb index b7dc1b7cf8..ece1a4912a 100644 --- a/Library/Homebrew/livecheck/strategy/sparkle.rb +++ b/Library/Homebrew/livecheck/strategy/sparkle.rb @@ -95,16 +95,16 @@ module Homebrew os = enclosure["os"].presence end - title = item.elements["title"]&.text&.strip&.presence - link = item.elements["link"]&.text&.strip&.presence + title = Xml.element_text(item, "title") + link = Xml.element_text(item, "link") url ||= link - channel = item.elements["channel"]&.text&.strip&.presence - release_notes_link = item.elements["releaseNotesLink"]&.text&.strip&.presence - short_version ||= item.elements["shortVersionString"]&.text&.strip&.presence - version ||= item.elements["version"]&.text&.strip&.presence + channel = Xml.element_text(item, "channel") + release_notes_link = Xml.element_text(item, "releaseNotesLink") + short_version ||= Xml.element_text(item, "shortVersionString") + version ||= Xml.element_text(item, "version") minimum_system_version_text = - item.elements["minimumSystemVersion"]&.text&.strip&.gsub(/\A\D+|\D+\z/, "")&.presence + Xml.element_text(item, "minimumSystemVersion")&.gsub(/\A\D+|\D+\z/, "") if minimum_system_version_text.present? minimum_system_version = begin MacOSVersion.new(minimum_system_version_text) @@ -113,7 +113,7 @@ module Homebrew end end - pub_date = item.elements["pubDate"]&.text&.strip&.presence&.then do |date_string| + pub_date = Xml.element_text(item, "pubDate")&.then do |date_string| Time.parse(date_string) rescue ArgumentError # Omit unparsable strings (e.g. non-English dates) diff --git a/Library/Homebrew/livecheck/strategy/xml.rb b/Library/Homebrew/livecheck/strategy/xml.rb index d78c5f0574..128db596ba 100644 --- a/Library/Homebrew/livecheck/strategy/xml.rb +++ b/Library/Homebrew/livecheck/strategy/xml.rb @@ -72,6 +72,30 @@ module Homebrew end end + # Retrieves the stripped inner text of an `REXML` element. Returns + # `nil` if the optional child element doesn't exist or the text is + # blank. + # @param element [REXML::Element] an `REXML` element to retrieve text + # from, either directly or from a child element + # @param child_path [String, nil] the XPath of a child element to + # retrieve text from + # @return [String, nil] + sig { + params( + element: REXML::Element, + child_path: T.nilable(String), + ).returns(T.nilable(String)) + } + def self.element_text(element, child_path = nil) + element = element.get_elements(child_path).first if child_path.present? + return if element.nil? + + text = element.text + return if text.blank? + + text.strip + end + # Parses XML text and identifies versions using a `strategy` block. # If a regex is provided, it will be passed as the second argument to # the `strategy` block (after the parsed XML data). diff --git a/Library/Homebrew/test/livecheck/strategy/xml_spec.rb b/Library/Homebrew/test/livecheck/strategy/xml_spec.rb index 51cc36f923..440147a2a6 100644 --- a/Library/Homebrew/test/livecheck/strategy/xml_spec.rb +++ b/Library/Homebrew/test/livecheck/strategy/xml_spec.rb @@ -79,6 +79,24 @@ describe Homebrew::Livecheck::Strategy::Xml do EOS end + let(:parent_child_text) { { parent: "1.2.3", child: "4.5.6" } } + let(:content_parent_child) do + # This XML deliberately includes unnecessary whitespace, to ensure that + # Xml#element_text properly strips the retrieved text. + <<~EOS + + + + #{parent_child_text[:parent]} + #{parent_child_text[:child]} + + + + + + EOS + end + let(:content_matches) { ["1.1.2", "1.1.1", "1.1.0", "1.0.3", "1.0.2", "1.0.1", "1.0.0"] } let(:content_simple_matches) { ["1.2.3"] } @@ -123,6 +141,29 @@ describe Homebrew::Livecheck::Strategy::Xml do end end + describe "::element_text" do + let(:parent_child_doc) { xml.parse_xml(content_parent_child) } + let(:parent) { parent_child_doc.get_elements("/elements/parent").first } + let(:blank_parent) { parent_child_doc.get_elements("/elements/blank-parent").first } + + it "returns the element text if child_name is not provided" do + expect(xml.element_text(parent)).to eq(parent_child_text[:parent]) + end + + it "returns the child element text if child_name is provided" do + expect(xml.element_text(parent, "child")).to eq(parent_child_text[:child]) + end + + it "returns `nil` if the provided child element does not exist" do + expect(xml.element_text(parent, "nonexistent")).to be_nil + end + + it "returns `nil` if the retrieved text is blank" do + expect(xml.element_text(blank_parent)).to be_nil + expect(xml.element_text(blank_parent, "blank-child")).to be_nil + end + end + describe "::versions_from_content" do it "returns an empty array when given a block but content is blank" do expect(xml.versions_from_content("", regex) { "1.2.3" }).to eq([])