brew icon indicating copy to clipboard operation
brew copied to clipboard

Load internal cask json v3

Open apainintheneck opened this issue 3 months ago • 6 comments

  • [x] Have you followed the guidelines in our Contributing document?
  • [x] Have you checked to ensure there aren't other open Pull Requests for the same change?
  • [x] Have you added an explanation of what your changes do and why you'd like us to include them?
  • [x] Have you written new tests for your changes? Here's an example.
  • [x] Have you successfully run brew style with your changes locally?
  • [x] Have you successfully run brew typecheck with your changes locally?
  • [ ] Have you successfully run brew tests with your changes locally?

This is the follow-up to https://github.com/Homebrew/brew/pull/16798 where we start loading the cask from the new JSON API. It ended up being quite similar in to what I did in https://github.com/Homebrew/brew/pull/16638 a few weeks ago.

apainintheneck avatar Mar 15 '24 08:03 apainintheneck

Looks good! Again: avoiding the if/else might be nice if possible but fine to merge as-is. Thanks!

The ones in Homebrew::API::Cask and Tap are kind of unavoidable unless we want to change how we organize code a lot. This also mimics how the formula logic already works. The logic in the cask loader can maybe be improved though.

apainintheneck avatar Mar 16 '24 04:03 apainintheneck

It looks like there is still broad disagreement about which types of cask files should get stored during installation, how to prioritize backwards compatibility with legacy cask file formats and which shape any new formats should have going forward. For that reason, I will close this PR and revert the changes made in previous PRs to generate cask JSON v3. At some future point, or as this discussion progresses it might be worth revisiting this idea but now is not that time.

  • To revert
    • https://github.com/Homebrew/formulae.brew.sh/pull/1092
    • https://github.com/Homebrew/brew/pull/16798
  • Related discussion
    • https://github.com/Homebrew/brew/issues/17013

apainintheneck avatar Apr 10 '24 02:04 apainintheneck

Do we care at all about potentially having formula JSON v3 and cask JSON v2 at the same time? They don't technically rely on each other.

apainintheneck avatar Apr 10 '24 03:04 apainintheneck

@apainintheneck Please don't close this, I can assign this to myself and finish it off. If you'd rather someone else opened this PR and it includes your commit(s): I can do that and use that as building blocks for my own PR(s).

MikeMcQuaid avatar Apr 10 '24 07:04 MikeMcQuaid

Feel free to use this PR as a starting point. I don't currently see any path forward that doesn't require first merging in other changes to the codebase. By that time this PR will likely be more than a month out-of-date. There is also no agreement currently on what we want this to look like which is why it made sense for me to close this. I'd be interested to hear your point of view on what you think it will take to get this merged in.

apainintheneck avatar Apr 11 '24 01:04 apainintheneck

Feel free to use this PR as a starting point.

Thanks 👍🏻

MikeMcQuaid avatar Apr 11 '24 07:04 MikeMcQuaid