cmd/aspell-dictionaries: fix typecheck failure

[chenrui333]

• Have you followed the guidelines for contributing?
• Have you ensured that your commits follow the commit style guide?
• Have you checked that there aren't other open pull requests for the same formula update/change?
• Have you built your formula locally with HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
• Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
• Does your build pass brew audit --strict <formula> (after doing HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

---

seeing some typecheck failure as

  /home/linuxbrew/.linuxbrew/Homebrew/Library/Taps/homebrew/homebrew-core/cmd/aspell-dictionaries.rb:26: Method `split` does not exist on `SystemCommand::Result` https://srb.help/7003
      26 |        index_output.split("<tr><td>").each do |line|
                               ^^^^^
    Got `SystemCommand::Result` originating from:
      /home/linuxbrew/.linuxbrew/Homebrew/Library/Taps/homebrew/homebrew-core/cmd/aspell-dictionaries.rb:25:
      25 |        index_output, = Utils::Curl.curl_output("#{dictionary_url}/0index.html")
                  ^^^^^^^^^^^^
    Did you mean `plist`? Use `-a` to autocorrect
      /home/linuxbrew/.linuxbrew/Homebrew/Library/Taps/homebrew/homebrew-core/cmd/aspell-dictionaries.rb:26: Replace with `plist`
      26 |        index_output.split("<tr><td>").each do |line|
                               ^^^^^
      ./system_command.rb:432: Defined here
       432 |    def plist
                ^^^^^^^^^
  Errors: 1
  Check https://docs.brew.sh/Typechecking for more information on how to resolve these errors.

relates to Homebrew/brew#19077

[samford]

Thanks! I did a quick search and it looks like this is the only use of a Utils::Curl method in homebrew/core.

[samford]

How this command handles parsing seems a bit fragile (it may break if the HTML slightly changes). One moment and I'll see if I can improve this (without using T.must).

Edit: I ended up with one T.must call but only because Sorbet doesn't distinguish between required and optional capture groups in regex matches. Values for required capture groups will always exist if the regex matches but Sorbet can't understand that, so you have to either use T.must or handle the values as if it's possible for them to be nil.

[chenrui333]

that works for me 👍

[samford]

I spent more time on this than necessary but I reworked the language-finding logic to use a regex, extracting the language and path information from the href attributes that we want. The path URL contains the language (as the directory name), so we can capture both values from the href attribute (i.e., no need to check multiple table cells). You can see this in my branch here: https://github.com/samford/homebrew-core/commits/rework-aspell-dictionaries/

This minimizes our reliance on an overly-specific HTML structure and it uses case-insensitive matching, so it will continue to work if the capitalization of HTML tags/attributes changes or the table cell order changes.

We can technically match within href attributes without worrying about table cells at all (i.e., omitting the leading <td[^>]*?>.* from the regex) but there's one link on the page with a .txt extension that can match if we're not strict about the tarball filename format (or extension). I've made the regex strict enough that it works without matching table cells but I've left that in for now. If we run into issues in the future, it's easy to make changes to the regex with this setup.

The new approach can still fail if the file type changes to something other than a tarball or if the path changes in a way that the regex won't match (or handle correctly). It's not perfect but this approach may be less brittle than the existing logic.

Besides that, I made some changes to raise on error conditions instead of quietly failing with no indication of what went wrong.

If you're good with this approach, feel free to pull the commits from my branch or I can push them here. The first commit covers minimal changes that are necessary here for brew typecheck to past and the other two are refactoring/improvements.

[chenrui333]

yeah, I would just think we should get the CI unblocked first and then do a followup PR on the rework, what do you think?

[samford]

I would just think we should get the CI unblocked first and then do a followup PR on the rework

The rework may need some review, so that makes sense to me 👍