Refactor livecheck strategy block handling

[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 PR does a fair bit of refactoring/reworking of parts of livecheck's Strategy module (and the strategies within) with the following goals:

• Rework how strategy constants are identified, so we don't have to make other Strategy constants private
• Standardize valid return values for strategy blocks across all strategies, to further simplify the mental model of strategy block return values
• Refactor strategy block handling code into methods, to allow this code to be tested (without needing to make a network request or stubbing at this point)
• Improve documentation comments, add missing method signatures, etc. (general clean up)

Rework how strategy constants are identified

Up to this point, we've had to rely on making Strategy constants private to ensure that the only available constants are strategies. With the current setup, the existence of a constant that's not a strategy would break Strategy#strategies and Livecheck#livecheck_strategy_names.

Instead, we can achieve the same goal by skipping over constants that aren't a class. Other than saving us from having to make these constants private, this is necessary to be able to create a Strategy constant that can be used in all strategies.

Standardize valid strategy block return types

Valid strategy block return types currently vary between strategies. Some only accept a string whereas others accept a string or array of strings. [strategy blocks also accept a nil return (to simplify early returns) but this was already standardized across strategies.]

While some strategies only identify one version by default (where a string is an appropriate return type), it could be that a strategy block identifies more than one version. In this situation, the strategy would need to be modified to accept (and work with) an array from a strategy block.

Rather than waiting for this to become a problem, this modifies all strategies to standardize on allowing strategy blocks to return a string or array of strings (even if only one of these is currently used in practice). Standardizing valid return types helps to further simplify the mental model for strategy blocks and reduce cognitive load.

This commit extracts related logic from #find_versions into methods like #versions_from_content, which is conceptually similar to PageMatch#page_matches (renamed to #versions_from_content for consistency). This allows us to write tests for the related code without having to make network requests (or stub them) at this point. In general, this also helps to better align the structure of strategies and how the various #find_versions methods work with versions.

There's still more planned work to be done here but this is a step in the right direction.

Looking forward

After this PR, I'll be creating a follow-up PR to extract the code that generates a strategy URL and regex into a separate method (#generate_input_values). This will allow us to test this code as well but, more importantly, having strategy URL generation in a method is a necessary step for creating an intelligent caching setup. More on that later but this should be one of the last remaining requirements to be able to implement caching (though there are some other areas that I'm working on before that).

I'll also be creating a follow-up PR to refactor the arguments to the #find_versions methods, as they've become a bit unruly over time and some forthcoming PRs would make it even worse. The goal is to not have to modify every #find_versions method if one strategy needs a new parameter (e.g., cask). This should tidy up the method parameters and make future development a bit less onerous.

[BrewTestBot]

Review period will end on 2021-08-12 at 01:09:05 UTC.

[MikeMcQuaid]

Scanned but seems reasonable to me. CC @maxim-belkin for more livecheck context thoughts.

[maxim-belkin]

I like most of the changes: code reuse, improved comments, method signatures. I do have a couple of comments/questions:

1. Comment: all self.versions_from_content methods have common if block... statement that can be factored out into its own method.
2. Minor concern: invalid_url is a very generic name (for a variable) and might be a tiny bit misleading because it stores not an invalid URL per se but the one that that particular method does not expect/can not process. So, personally, I'd prefer to keep old names (e.g., non_gnu_url, non_gnome_url, etc) but this is not a deal breaker for me. Related to this: message returns false for an invalid URL might be slightly misleading for the same reason.

[samford]

all self.versions_from_content methods have common if block... statement that can be factored out into its own method.

I was honestly thinking the same thing now that these are all the same (outside of the block#call argument(s)). I'll work on adding this in 👍

invalid_url is a very generic name (for a variable) and might be a tiny bit misleading because it stores not an invalid URL per se but the one that that particular method does not expect/can not process. So, personally, I'd prefer to keep old names (e.g., non_gnu_url, non_gnome_url, etc) but this is not a deal breaker for me. Related to this: message returns false for an invalid URL might be slightly misleading for the same reason.

The potential ambiguity of invalid_url (and related messaging) crossed my mind as I was working on it, so I can understand the concern. I'm good with returning to the previous (more descriptive) names and it makes sense to me.

[samford]

The latest push adds a #handle_block_return method to Strategy and replaces related code in strategies with a statement like return Strategy.handle_block_return(block.call(content, regex)) if block.

This also reworks the previous test changes to return to using variable names like non_gnu_url and to standardize on this naming convention.

---

It probably goes without saying at this point but I've done comparative runs across core/cask and didn't see any regressions.

[BrewTestBot]

Review period ended.

[maxim-belkin]

Thank you, Sam!