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

style: forbid url do blocks in homebrew/cask #18404

Merged
merged 2 commits into from
Sep 25, 2024

Conversation

Rylan12
Copy link
Member

@Rylan12 Rylan12 commented Sep 25, 2024

Follow-up to #18390

This PR adds a rubocop check that forbids the use of url do blocks in homebrew/cask. It still allows them to be used in non-official taps. There's a little bit more discussion/context in the PR linked above, but basically these blocks require us to make network calls every time we generate the API data. It's very common for the API generation to fail because one of these URL blocks has an issue, and really the generation should be able to happen offline.

This will need to wait for Homebrew/homebrew-cask#186501 to be merged

CC: @Homebrew/cask

@Rylan12 Rylan12 added cask Homebrew Cask install from api Relates to API installs labels Sep 25, 2024
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

This is great, thanks @Rylan12!

As a follow-up: I do think it's worth odeprecateing this functionality. It's both pretty niche and very likely to start breaking in future if we're not using it ourselves in any official taps.

@MikeMcQuaid
Copy link
Member

As a follow-up: I do think it's worth odeprecateing this functionality. It's both pretty niche and very likely to start breaking in future if we're not using it ourselves in any official taps.

To be explicit: this would ideally be a follow-up PR before we release Homebrew 4.4.0 but after this PR is merged.

@MikeMcQuaid
Copy link
Member

As a follow-up: I do think it's worth odeprecateing this functionality. It's both pretty niche and very likely to start breaking in future if we're not using it ourselves in any official taps.

Opened #18407 to attempt this.

@Bo98
Copy link
Member

Bo98 commented Sep 25, 2024

As a follow-up: I do think it's worth odeprecateing this functionality. It's both pretty niche and very likely to start breaking in future if we're not using it ourselves in any official taps.

I agree with the point about it being untested. Though equally I don't want taps to be encouraged to start doing network requests on cask-load like I've seen way too many third-party formulae do. url do at least did things properly in that regard - just unsuitable for JSON API where we don't run Ruby.

I'm overall neutral, though if we do deprecate would suggest very clear messaging in what the replacement is.

@MikeMcQuaid
Copy link
Member

Though equally I don't want taps to be encouraged to start doing network requests on cask-load like I've seen way too many third-party formulae do.

Those same taps will just end up complaining when we inevitably break this functionality because we don't use it ourselves. I'm not terribly worried about doing network requests on cask-load given this is not entirely dissimilar.

@MikeMcQuaid
Copy link
Member

though if we do deprecate would suggest very clear messaging in what the replacement is.

Will happily take suggestions for what this messaging should be. As far as I can tell: there is no real replacement here.

@Rylan12 Rylan12 merged commit 479eeaa into master Sep 25, 2024
27 checks passed
@Rylan12 Rylan12 deleted the forbid-url-do-homebrew-cask branch September 25, 2024 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cask Homebrew Cask install from api Relates to API installs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants