feat: Add interface for livecheck strategies

[dduugg]

• 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?

---

As suggested in the conversation in #19293, this PR introduces a Livecheck::Strategy interface, to clarify what methods a strategy must implement in order to fulfill the contract of a strategy. The first five strategies (alphabetically) extend the interface for illustrative purposes. (I can extend to the remainder in a follow-up PR.)

[samford]

Mostly just a few stylistic nits to align with the rest of livecheck but this generally makes sense to me.

Referring back to our previous discussion: if we ever require all [third-party] strategies to extend Strategic, this would allow us to use T::Class[Strategic] instead of T::Class[T.anything] to refer to livecheck strategies in type signatures, correct?

Some general things to note, if it affects this:

• url is generally treated as a required parameter but it's optional for ExtractPlist (cask is the required parameter there and url is optional).
• provided_content will be renamed to content and added to all find_versions methods except ExtractPlist in an upcoming PR (after Options is merged). content is usually T.nilable(String) but for some strategies it's a hash (GithubLatest) or array of hashes (GithubReleases, HeaderMatch). I'm considering using JSON.generate/JSON.parse to allow content to always be a string but that would introduce some unnecessary overhead simply for the sake of type uniformity, so I'm still undecided.
• The homebrew_curl parameter is currently only present in find_versions methods where page_content or page_headers is called. However, this will be replaced by Options, so we can similarly replace homebrew_curl/unused here once that's merged.
• I'm working on changes to migrate as much of livecheck to typed: strong as possible (leaving livecheck/livecheck.rb until we've taken care of some planned refactoring) and after that I'm going to try to tackle T.untyped. I'm replacing some usage as part of the typed: strong changes but the find_versions return hash may be a challenge, as it has a number of variations (e.g., default, default with merged values from page_content, default with cached boolean, etc.). It seems like it should be technically possible but accounting for the variations may be a chore (depending on what Sorbet allows).

[dduugg]

@samford PTAL

I've extended the interface to all strategies (and i've also done some light refactoring). Totally fine to adjust it as you evolve find_versions: it isn't public API, and IMO it's actually preferable to have a single canonical place to lookup the current API, if it's in a state of flux.

re T::Class[Strategic], we should still be able to use it, even without third-party implementations. Sorbet doesn't do runtime member checking of T:: types for performance reasons (e.g. T::Array[String]), so we should only have to pass static analysis:

class Foo
  sig { params(foo: T::Class[Numeric]).returns(T::Class[Numeric]) }
  def bar(foo); foo; end
end
Foo.new.bar(String)
# => String

[samford]

One small issue to address and a couple stylistic nits but this is looking good. That said, there's one thing I would like to discuss before merging (see below).

I'm short on time this morning but I'll test this with a tap strategy later today, to make sure everything continues to work as expected [without extending Strategic].

---

I'm still not sure about the Strategic name and I'm curious what others think. I understand that it aligns with Numeric but it doesn't feel like the same kind of terminological relationship to me. For example, "Integer is a Numeric class" makes sense but "PageMatch is a Strategic class" seems odd/vague to me.

As mentioned before, Strategy would be ideal but it's already used by the existing module. However, it looks like UnpackStrategy provides both abstract and concrete methods in one module. I've only glanced at that setup but would it make sense to do something similar here or would there be issues? I can try to look more into this later today if/when I have time.

Barring that, there may be something to be said for something like LivecheckStrategy (if we can't use Strategy), as it aligns with UnpackStrategy and we already have some precedent with LivecheckVersion.

I can live with Strategic if using Strategy isn't feasible and I'm the only one who has an issue with the name (compared to something like LivecheckStrategy) but I think it's something we should at least discuss (i.e., we can't easily change it once it's used in third-party strategies, so best to make sure we get it right from the start).

[dduugg]

One small issue to address and a couple stylistic nits but this is looking good. That said, there's one thing I would like to discuss before merging (see below).

I'm short on time this morning but I'll test this with a tap strategy later today, to make sure everything continues to work as expected [without extending Strategic].

I'm still not sure about the Strategic name and I'm curious what others think. I understand that it aligns with Numeric but it doesn't feel like the same kind of terminological relationship to me. For example, "Integer is a Numeric class" makes sense but "PageMatch is a Strategic class" seems odd/vague to me.

As mentioned before, Strategy would be ideal but it's already used by the existing module. However, it looks like UnpackStrategy provides both abstract and concrete methods in one module. I've only glanced at that setup but would it make sense to do something similar here or would there be issues? I can try to look more into this later today if/when I have time.

Barring that, there may be something to be said for something like LivecheckStrategy (if we can't use Strategy), as it aligns with UnpackStrategy and we already have some precedent with LivecheckVersion.

I can live with Strategic if using Strategy isn't feasible and I'm the only one who has an issue with the name (compared to something like LivecheckStrategy) but I think it's something we should at least discuss (i.e., we can't easily change it once it's used in third-party strategies, so best to make sure we get it right from the start).

er, Numeric is a class, though it's the exception that justifies the rule: "An Integer is a Numeric" (class/noun wording) isn't proper English, though "An Integer is Numeric" (module/adjective wording) is. Modules/interfaces are meant to be a list of non-hierarchical behaviors/properties (e.g. a Foo is Enumerable/Comparable/Magical), so Strategic, while not perfect, still makes the most sense to me of the above options. (Livecheckable could maybe work, but I think of that as a Formula property.)

(IIRC Rails has some silly ones like Deduplicable, MissingAttachable, Placeholderable – the word choice is less important than communicating the implemented behavior.)

Sorbet interfaces can't implement methods, so I don't think it can share the Strategy namespace (and I also think it provides better separation of concerns not to mix them).

This isn't public API (yet) though, so I can leave a comment in the interface to revisit before making public, if you'd like.

[samford]

My issue is that I don't think "strategic" carries the same level of meaning as "numeric", "enumerable", "comparable", etc. Those are all concepts that are readily understood but it's ambiguous what "strategic" could mean from the name alone. However, anyone encountering Strategic in livecheck code may already have enough context to have a rough idea (and the module comment explains the rest), so it may just be one of those things that I'll get used to over time.

Livecheckable could maybe work, but I think of that as a Formula property.

Fwiw, I've previously taken steps to remove/avoid "livecheckable" as a term. Prior to 2020, livecheck lived in the homebrew-livecheck repository, so all the checks were separate from related formulae. The livecheck calls existed in standalone files with the same name as the formula and these files were called "livecheckables". As a result, the #livecheckable? method was basically intended to mean "formula/cask/resource has a livecheck block" but that relies on historical context, so we renamed it to #livecheck_defined?.

Sorbet interfaces can't implement methods, so I don't think it can share the Strategy namespace (and I also think it provides better separation of concerns not to mix them).

UnpackStrategy is using abstract! instead of interface! to account for that but I kind of agree that keeping the interface separate is probably nicer.

This isn't public API (yet) though, so I can leave a comment in the interface to revisit before making public, if you'd like.

Not sure if a comment is necessary but [assuming making it public will happen in another PR] feel free to ping me, request my review, etc.

[samford]

Thanks, @dduugg! Previous comments aside, this is some nice work. I'll be sure to update this as I make changes to find_versions methods in an upcoming PR 👍