-
-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
livecheck: error on invalid url symbol #18622
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
6 tasks
p-linnane
approved these changes
Oct 24, 2024
carlocab
approved these changes
Oct 24, 2024
samford
force-pushed
the
livecheck/error-on-invalid-url-symbol
branch
from
October 25, 2024 02:45
9eeefc6
to
12ccae8
Compare
This latest push includes the following changes:
|
MikeMcQuaid
reviewed
Oct 25, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @samford, a few thoughts.
Up to now, we haven't been accounting for `#url` symbol arguments in `livecheck` blocks that don't reference a checkable URL. This can either be an invalid symbol (e.g., using the `:stable` formula symbol in a cask) or a valid symbol where the referenced URL doesn't exist (e.g., using `:head` when there's no `head` URL). [Almost all of the valid symbols are required URLs but `head` is optional.] Over the years, we've had a handful of slips where we've used `:url` in formulae (when it's only valid in casks) and `:stable` in casks (when it's only valid in formulae). In this scenario, `livecheck_url_string` is `nil`, so livecheck falls back to `#checkable_urls`. In this scenario, `stable` and `url` are the first checkable URLs for formulae and casks (respectively), so the checks ended up working as expected merely by chance. This isn't obvious in the output and even the debug output looks normal. It only becomes apparent that livecheck isn't working as expected if it iterates through more than one checkable URL before reaching one that works (not the case in those instances). With that in mind, this adds an error when a `#url` symbol is used but it doesn't correspond to a checkable URL. This will account for both of the mentioned scenarios (invalid symbols and valid ones referencing a non-existent URL) and prevent livecheck from quietly proceeding in an unexpected manner.
This expands tests for `#checkable_urls` to cover everything except branches that shouldn't ever be reached.
samford
force-pushed
the
livecheck/error-on-invalid-url-symbol
branch
from
October 25, 2024 14:26
12ccae8
to
bfdb84f
Compare
MikeMcQuaid
approved these changes
Oct 25, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good thanks @samford! One suggested refactoring.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?Up to now, we haven't been accounting for
#url
symbol arguments inlivecheck
blocks that don't reference a checkable URL. This can either be an invalid symbol (e.g., using the:stable
formula symbol in a cask) or a valid symbol where the referenced URL doesn't exist (e.g., using:head
when there's nohead
URL). [Almost all of the valid symbols are required URLs buthead
is optional.]Over the years, we've had a handful of slips where we've used
:url
in formulae (when it's only valid in casks) and:stable
in casks (when it's only valid in formulae). In this scenario,livecheck_url_string
isnil
, so livecheck falls back to#checkable_urls
.stable
andurl
are the first checkable URLs for formulae and casks (respectively), so the checks ended up working as expected merely by chance. This isn't obvious in the output and even the debug output looks normal. It only becomes apparent that livecheck isn't working as expected if it iterates through more than one checkable URL before reaching one that works (not the case in those instances).With that in mind, this adds an error when a
#url
symbol is used but it doesn't correspond to a checkable URL. This will account for both of the mentioned scenarios (invalid symbols and valid ones referencing a non-existent URL) and prevent livecheck from quietly proceeding in an unexpected manner.It's worth mentioning that I will also be creating a RuboCop for this in a later PR, so we can catch this with
brew style
/brew audit
(not just at runtime).I'm currently reworking the livecheck RuboCops to share cops between formulae, resources, and casks. We originally implemented them as
FormulaCop
s beforelivecheck
blocks were available in casks (and laterresource
blocks) but we didn't have a way of sharing cops at the time. We now have some shared cops, so it's just a matter of migrating the cops to that pattern. I have most of that done (it works for formulae/resources and most casks) but I'm working on writing tests and handling less common cask setups. Once that's done, I'll create a PR for the refactor and added RuboCops.