Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor #find_versions parameters in strategies #11854

Merged

Conversation

samford
Copy link
Member

@samford samford commented Aug 12, 2021

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

Livecheck's strategies all contain a #find_versions method that takes input (e.g., a url, regex, cask, etc.) and uses it to identify versions from a source. All of the strategies except for ExtractPlist expect that a URL will be provided. ExtractPlist is an exception (in many unfortunate ways), as it expects a cask instead of a URL.

When ExtractPlist was created, a cask parameter was added to every #find_version method. This is because livecheck uses all the possible arguments when it calls the method, regardless of the strategy (i.e., it doesn't cater the arguments to what the strategy expects). If a strategy didn't have the cask parameter but a named cask argument was provided, Ruby would give an error.

Ideally, a given #find_versions method should only be required to have parameters for what it actually uses and we shouldn't have to update every method if one strategy needs a new parameter. This PR aims to achieve this by removing unused parameters from #find_versions methods and including an **unused parameter in each, as the double splat will take care of any arguments that don't correspond to a named parameter for a given strategy.

For example, ExtractPlist has a named cask: parameter (since it's the only strategy that works with a cask) but all the others omit this. Instead of producing an error, the cask parameter is simply part of unused (i.e., unused[:cask]).

In general, this helps to minimize #find_versions parameters to only what's used. This will make it easier to add new strategies in the future, where a new parameter wouldn't need to be added to all other strategies. Besides that, this bit of clean up helps to make things a bit more manageable before I add a couple more parameters to most #find_versions methods in the near future.


As usual, I've done comparative before/after livecheck runs across first-party formulae/casks and I didn't see any regressions.

@BrewTestBot
Copy link
Member

Review period will end on 2021-08-13 at 22:30:10 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Aug 12, 2021
@samford samford force-pushed the livecheck/refactor-find_versions-parameters branch from 4bf9622 to bdac98e Compare August 12, 2021 22:55
@BrewTestBot
Copy link
Member

Review period ended.

@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Aug 14, 2021
@samford samford force-pushed the livecheck/refactor-find_versions-parameters branch from bdac98e to 13b349b Compare August 16, 2021 16:02
Copy link
Member

@maxim-belkin maxim-belkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proposed changes seem reasonable to me: this PR transforms some of the required arguments to keyword arguments and hides the cask keyword argument inside the unused hash, where appropriate. I have one question (see the apache.rb), but overall 👍🏻 from me.

Library/Homebrew/livecheck/strategy/apache.rb Show resolved Hide resolved
@samford samford merged commit f026dd2 into Homebrew:master Aug 17, 2021
@samford samford deleted the livecheck/refactor-find_versions-parameters branch August 17, 2021 21:32
@github-actions github-actions bot added the outdated PR was locked due to age label Sep 19, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants