Replace Nokogiri with REXML

[samford]

• Have you followed the guidelines in our Contributing document?
• Have you checked to ensure there aren't other open Pull Requests for the same change?
• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you written new tests for your changes? Here's an example.
• Have you successfully run brew style with your changes locally?
• Have you successfully run brew typecheck with your changes locally?
• Have you successfully run brew tests with your changes locally?

---

This is a follow-up to #11599, where I said I would replace use of nokogiri for XML parsing with rexml in parts of brew outside of the Sparkle livecheck strategy. These changes are generally a straightforward shift from nokogiri methods to rexml methods.

One notable difference is that rexml requires you to access attribute values using square bracket syntax instead of a method (like #attr in nokogiri).rexml has an #attribute method but it returns the full attribute (e.g., a="b"). We can't use a safe navigation operator in this context, so it was necessary to create an intermediate variable to be able to guard against nil values.

If we had more code doing this, I would say that we could probably just extend rexml to add an attr method that's just a wrapper for the square bracket syntax. This would allow us to use the safe navigation operator but I'm not sure if it's worth deviating from rexml's established API.

---

I'm not familiar with the BundleVersion and UnversionedCaskChecker classes but I've tried to exercise this using livecheck's ExtractPlist and Sparkle strategies, which use one or both of these classes. The output from checking casks that use these strategies was the same with and without these changes.

The other area where UnversionedCaskChecker is used is in the brew bump-unversioned-casks dev-cmd. I let this run for a few casks and it seemed to work as expected. I don't anticipate this introducing any issues but it wouldn't hurt for some homebrew/cask folks to test this out, as I'm not as knowledgeable about that side of Homebrew.

[BrewTestBot]

Review period will end on 2021-06-29 at 00:10:57 UTC.

[MikeMcQuaid]

Great work here! May be worth considering a brew style check (in a follow-up PR) that ensures there's no require "nokogiri" usage in Homebrew/brew.

[BrewTestBot]

Review period ended.

[samford]

May be worth considering a brew style check (in a follow-up PR) that ensures there's no require "nokogiri" usage in Homebrew/brew.

If we don't have any intention to keep nokogiri around for HTML parsing (this isn't necessary at the moment), should we simply remove it from the Gemfile? It was only added seven months ago and for these particular areas that we're replacing with rexml.

If we were to drop it from the Gemfile, would we still need/want a brew style check (e.g., to discourage it from being re-added) or no?

[MikeMcQuaid]

If we don't have any intention to keep nokogiri around for HTML parsing (this isn't necessary at the moment), should we simply remove it from the Gemfile? It was only added seven months ago and for these particular areas that we're replacing with rexml.

If we were to drop it from the Gemfile, would we still need/want a brew style check (e.g., to discourage it from being re-added) or no?

Yes, I think it can/should be dropped from the Gemfile. If it's not being pulled in as a recursive dependency: we can probably do nothing. My only concern is that it just gets re-added at some point in future and a brew style check that says what to do instead may help a bit more? Definitely not blocking and very much optional, though.

[samford]

Yes, I think it can/should be dropped from the Gemfile. If it's not being pulled in as a recursive dependency: we can probably do nothing.

nokogiri will still be installed as a dependency of mechanize but dropping it from the Gemfile (as a direct dependency) at least gets us back to where we were before it was added. I'll create a follow-up PR for the Gemfile/Gemfile.lock changes after I merge this.

My only concern is that it just gets re-added at some point in future and a brew style check that says what to do instead may help a bit more?

Certainly understandable to me. Last question: would we want the style check to also disallow gem "nokogiri" (in addition to require "nokogiri") or is the Gemfile out of scope?

Definitely not blocking and very much optional, though.

I'll try to tinker with this when I get a chance but, at the moment, there's some important livecheck work that I would like to focus on wrapping up into a PR first.

[MikeMcQuaid]

Certainly understandable to me. Last question: would we want the style check to also disallow gem "nokogiri" (in addition to require "nokogiri") or is the Gemfile out of scope?

Up to you!

I'll try to tinker with this when I get a chance but, at the moment, there's some important livecheck work that I would like to focus on wrapping up into a PR first.

👍🏻 thanks for all your work here and on livecheck!