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

Generate internal cask json v3 #16514

Conversation

apainintheneck
Copy link
Contributor

  • 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 sister PR to #16460. It start generating the internal_cask_v3.json file in the brew generate-cask-api file.

I didn't add any tests because they'll be easier to add later on when working on the cask loading code in the near future. The main thing here is that this doesn't break the brew generate-cask-api command which is easy to test.

I took the same approach as the other PR to cache the variations generation so the runtime of this command should be similar to what it was before.

The file size difference is significant but the old file wasn't as large as with formulae. It reduces the size from around 6MB to 4MB.

Related to: #16410

This is a slimmer JSON representation that omits unnecessary fields.

The goal here is to reduce download file size and JSON loading times.
This represents the state of the core cask tap when this got generated.
@apainintheneck apainintheneck added the install from api Relates to API installs label Jan 21, 2024
@apainintheneck apainintheneck marked this pull request as draft January 21, 2024 07:55
@apainintheneck
Copy link
Contributor Author

Moving back to draft since the discussion in #16460 is relevant for this PR and could result in changes to naming among other things.

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.

Looks good so far, thanks!

api_hash = to_hash_with_variations.except(
# Included in the top-level of the `internal_cask_v3.json` file
# created by the `brew generate-cask-api` command.
"old_tokens",
Copy link
Member

Choose a reason for hiding this comment

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

  • Unless I'm mistaken: token and full_token are always identical, too.
  • url_specs, languages seem to be often empty.
  • stuff like appcast is often null
  • installed and installed_time and outdated are reflective of the machine they are run on and not public API.

Copy link
Member

Choose a reason for hiding this comment

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

appcast is deprecated, if it's not null it should be.

@apainintheneck
Copy link
Contributor Author

Closing because the approach in the formula PR changed significantly which means this will need to be reworked.

@github-actions github-actions bot added the outdated PR was locked due to age label Feb 25, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
install from api Relates to API installs outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants