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

PageMatch: Expand #find_versions tests #11889

Conversation

samford
Copy link
Member

@samford samford commented Aug 19, 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?

This is a follow-up to #11888, where I modified the early return conditions for PageMatch#find_versions to allow a strategy block to [temporarily] act as a substitution for a regex. That PR was a fix for an issue affecting some casks that don't follow established patterns, so I wanted to get it merged quickly. This PR contains related changes that I would have handled in #11888 if I had more time.

This adds a test to ensure that PageMatch#find_versions works when a block is provided but a regex isn't. I've added comments to explain that this is temporary and the pattern in the test shouldn't be followed.

Similarly, I've updated related method signatures and documentation comments to reflect that the regex parameter is technically nilable for now. The method signature changes were required for this to pass.

Looking forward

Existing cask livecheck blocks that don't use #regex to establish a regex and pass it into a strategy block should be updated to do so. This has long been the pattern in homebrew/core and homebrew/cask should be brought up to standard (in many other ways as well).

One challenge is that there are some cask strategy blocks that require more than one regex but #regex currently only works with one regex. With some of these strategy blocks, there's a clear primary regex that can be established using #regex and any secondary regexes can be inlined in the strategy block. We do this with a few complex strategy blocks in homebrew/core (e.g., bash).

The issue is that some of the cask strategy blocks with more than one regex don't have a primary regex that can be reasonably extracted into a #regex call. These exceptions would prevent us from eventually getting to a point where we standardize on always establishing regexes using #regex and passing it into a strategy block.

One solution to this could be to modify the #regex DSL to allow either a hash with Regexp values or an array of Regexps. These would both achieve the same purpose but there are benefits/detriments to each.

Either way, we would only allow a non-Regexp value when a strategy block is present (as the strategy wouldn't know what to do with a non-Regexp value by default). However, we want #regex to appear before #strategy in a livecheck block (to make it clear that it's the same regex being passed into the strategy block), so this would need to be enforced somewhere outside of the #regex DSL method.

Allowing for multiple regexes when using #regex would also let us avoid having to determine which regex is a primary regex and which are secondary. This is currently a bit subjective, so it would be nice to avoid this question entirely.

This shouldn't be difficult to implement, so I'll investigate it and work on creating a PR.

@BrewTestBot
Copy link
Member

Review period will end on 2021-08-20 at 15:21:16 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Aug 19, 2021
@samford samford added the critical Critical change which should be shipped as soon as possible. label Aug 19, 2021
@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Aug 19, 2021
@BrewTestBot
Copy link
Member

Review period skipped due to critical label.

@samford
Copy link
Member Author

samford commented Aug 19, 2021

I'm labeling this as critical, since this is a continuation of #11888 and I didn't have time to thoroughly test that change. I'm going to do comparative runs across homebrew/core and homebrew/cask using the commit before #11862 was merged and the commit in this PR, to ensure everything is working as expected.

@samford
Copy link
Member Author

samford commented Aug 19, 2021

Having checked all the first-party formulae/casks, everything appears to be working as expected. Since #11888 wasn't thoroughly tested but this was, I'm going to go ahead and merge this as a continuation of #11888 after a quick rebase.

That should be it for critical bug fixes and I'll be returning to the usual PR review format for forthcoming changes.

@samford samford force-pushed the livecheck/PageMatch-expand-find_versions-tests branch from 317c23b to e65c9d0 Compare August 19, 2021 20:08
@samford samford merged commit 30e081b into Homebrew:master Aug 19, 2021
@samford samford deleted the livecheck/PageMatch-expand-find_versions-tests branch August 19, 2021 20:28
@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
critical Critical change which should be shipped as soon as possible. outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants