livecheck/strategy/extract_plist: enable livecheck url options #21335
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds functionality to pass livecheck URL options (such as cookies, headers, referer, user_agent) through to the ExtractPlist strategy when a custom URL is specified in a cask's livecheck block. The implementation iterates over livecheck options and copies matching values to the cask copy's URL specs.
Key Changes:
- Iterate through livecheck options and transfer them to the cloned cask's URL specs
- Only copy options that exist as keys in the cask URL specs
- Only set non-nil values
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| cask.livecheck.options.to_h.each do |spec| | ||
| next unless cask_copy.url.specs.key?(spec) | ||
|
|
||
| value = cask.livecheck.options.public_send(spec) | ||
| cask_copy.url.specs[spec] = value if value | ||
| end |
There was a problem hiding this comment.
The new functionality to pass livecheck URL options to the ExtractPlist strategy lacks test coverage. While tests exist in test/livecheck/strategy/extract_plist_spec.rb for the find_versions method with a custom URL (line 118-126), there are no tests verifying that livecheck options (cookies, header, referer, user_agent, etc.) are correctly transferred to the cask_copy.url.specs.
Consider adding tests that verify:
- Options from
livecheck.optionsare correctly copied tocask_copy.url.specs - The
:header/:headerskey mapping works correctly - Only non-nil values are transferred
- Options that don't exist in url.specs are skipped
bevanjkay
force-pushed
the
extractplist-url-options
branch
from
171a8b4 to
6fff2e7
Compare
There was a problem hiding this comment.
Nice catch! I totally forgot about ExtractPlist url overrides when I was working on the livecheck URL options.
I dug into this a bit (to confirm behavior) and noticed that this only works for user_agent. The default specs hash is { user_agent: :default } and cask_copy.url url resets the specs hash to default, so :user_agent is the only key that will pass the next unless cask_copy.url.specs.key?(options_key) guard. Even if we were comparing against the original URL's specs, this would only work when the original URL also sets a specific option (e.g., referer in both the cask url and livecheck block url). We'll have to approach this in a different way to allow using arbitrary livecheck block url options (and regardless of whether the original cask url already sets a similar option).
What we could do is collect the livecheck block url_options where the key has a corresponding keyword parameter in Cask::URL.initialize and then use those as arguments in the cask_copy.url url call. Restricting this to Options.url_options values will save us from having to unnecessarily iterate over non-URL options in the future and comparing to Cask::URL.initialize keyword parameters will ensure that we can set appropriate livecheck block URL values regardless of whether the existing cask url has the same arguments. Passing these values as url arguments also avoids directly setting specs values, which doesn't feel quite right (i.e., I don't think we do it anywhere else). This approach also addresses the :header keyword parameter vs. specs[:headers] naming issue that Copilot flagged, as we're only working with the initialize parameters (not specs values).
If this makes sense, I can push a commit once this suggestion has been reviewed and we've settled any technical/stylistic issues (this works but it may not be the best/prettiest way).
The previous commit intended to update the `ExtractPlist` strategy to also use `url` options from `livecheck` blocks when they're supported by `Cask::URL` (i.e., same keyword parameter name). However, due to the `next unless cask_copy.url.specs.key?(options_key)` guard, only the `user_agent` option worked because the `specs` hash includes a `user_agent` value by default. The prior `cask_copy.url url` call resets the `specs` hash to the default values, so options like `referer` were skipped even if the existing cask `url` had the same argument. This addresses the issue by collecting the keyword parameters of the `Cask::URL.initialize` method and comparing `livecheck` block `url` options to those names instead. This ensures that supported values will be used regardless of whether the existing cask `url` uses the same argument. Collecting keyword arguments also allows us to pass the values as arguments to the `cask_copy.url` call instead of setting `specs` values directly. Lastly, this addresses an issue in the previous approach where the `initialize` method used a `header` keyword parameter but the related specs key is `headers`. Besides that, this also uses `Options#url_options` instead of `Options#to_h`, as it will avoid unrelated options in the future (saving a little unnecessary work).
brew lgtm(style, typechecking and tests) with your changes locally?This change ensures that any
livecheck.url.optionsare passed through to theExtractPliststrategy when thelivecheckblock is given a different url. I have tried to use a robust approach that should automatically pick up any additional keys that may be provided in future, by checking thelivecheck.url.optionskeys against thecask.url.specsto see if they respond.CC: @samford