-
-
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: add support for casks #8578
Conversation
Great work. Would the |
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.
Very nice! I'd love @samford and @nandahkrishna to take a look before this is merged.
also implicitly using the :appcast if nothing is specified would make sense. |
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 @SeekingMeaning for this PR! Just a few minor comments. Also, would --formulae-only
and --casks-only
flags for the brew livecheck
developer command be useful?
I don't often find myself in homebrew-cask but I happened to be looking at a cask the other day and thinking about adding support to I'm in the process of reviewing and testing this and I'll have some feedback and requested changes once I'm finished. |
bd9a906
to
929f0a0
Compare
@miccal Yes
@core-code @nandahkrishna Done (I'm assuming all casks at least have
Yes, probably
Maybe, but the doc comment says "Returns a
@reitermarkus I actually did this initially but the |
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 for your work here, @SeekingMeaning.
There may be more room for improvement outside of my specific comments below but this is my first pass through this. Since this is a significant addition, I would prefer that we take our time with this to make sure it's in good shape before merging.
One thing that sticks out to me right now is the relative verbosity of the various formula_or_cask
language. It would be nice if we could find a generic way to refer to the concept of a formula or cask that's shorter but maybe this is just me.
There may be value in having a flag that allows us to restrict livecheck
to only work with either formula or casks. Maybe something like --type=formula
/--type=cask
or individual flags like --formula-only
/--cask-only
. This would allow users to only work with one or the other for a given run, which may be useful in conjunction with the --all
or --tap=
options. This should be relatively easy to handle, since it only affects the Livecheck#livecheck
method.
Edit: I just now saw Nanda's suggestion about the same flags, ahah. I'm not surprised we had the same idea.
From what I'm seeing, roughly 1/3 of the ~3645 casks have a URL that matches a strategy (~87% use the Git strategy) and a fair number of those produce an okay result. Hopefully we can look through the URLs and identify patterns that can be handled with additional strategies. If not, we're looking at 2500+ additional items to write livecheck blocks for, on top of the ~600 formulae that still need livecheck
blocks.
Yes, I agree — what about
I'll try my best |
a84fc1b
to
9429d74
Compare
😄 As for the flags, it seems |
List of casks without an automatic livecheck strategy (2,319 total): https://gist.github.com/SeekingMeaning/6c7a6d072a562c6ac474d5c0bb558076 |
I'm fine with this, personally. Would definitely prefer over anything like |
if you are looking for text patterns to find new versions of casks - i can contribute a few dozen of those. |
@SeekingMeaning, anything still missing here? |
@SeekingMeaning Would love to get this in in the nearish future. We can iterate on this in future PRs; I don't think this needs to be perfect to be merged. |
61bd307
to
738567f
Compare
Review period ended. |
With this having been rebased, I'll take another pass to review and test this as soon as I can. Thanks for helping to get this into shape, @reitermarkus. |
12bfb1f
to
935a9f4
Compare
I have removed |
- Skip blank lines in watchlist - Initialize Livecheck#version as nil - Simplify livecheck_version logic - Make test a bit more understandable
When running `brew livecheck --cask` or `brew livecheck --formula`, livecheck wasn't properly respecting these flags. It should have worked by only including the casks or formulae in the watchlist but instead these flags were treating all the names in the watchlist as formulae or casks, which doesn't work properly. This addresses the issue by always using `#to_formulae_and_casks` on the watchlist names and then using `#reject` to conditionally exclude formulae or casks based on whether the related flags were passed to `brew livecheck`.
d0ee4dc
to
15a868c
Compare
Thanks, everyone! |
brew style
with your changes locally?brew tests
with your changes locally?This allows
brew livecheck
to work on casks and adds a newlivecheck
stanza to the Cask DSL. Since many casks use methods such asversion.before_comma
, a newversion
field is added to theLivecheck
class that accepts either aString
or a validSymbol
(before_comma
,after_comma
,before_colon
,after_colon
)Example
livecheck
foransible-dk
: