-
-
Notifications
You must be signed in to change notification settings - Fork 10.7k
Add new combined internal JSON #21092
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR refactors the cask API generation code by extracting it into a reusable generate_cask_api! method in the Homebrew::API module, and adds functionality to generate a new combined internal JSON file that includes both formulae and casks data for each platform tag.
Key changes:
- Extracted cask API generation logic from
generate-cask-api.rbintoHomebrew::API.generate_cask_api!method - Added generation of combined
packages.#{bottle_tag}.jsonfiles containing both formulae and casks data - Added
strip_unneeded_internal_fieldsmethod (currently unused) to API module
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| Library/Homebrew/dev-cmd/generate-cask-api.rb | Simplified to delegate to the new Homebrew::API.generate_cask_api! method |
| Library/Homebrew/api.rb | Added generate_cask_api! method, cask_html_template helper, and strip_unneeded_internal_fields method |
| Library/Homebrew/dev-cmd/generate-formula-api.rb | Calls generate_cask_api! to retrieve cask data and generates combined packages.#{bottle_tag}.json files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Rylan12
left a comment
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.
I will take a closer look tomorrow, but I think this seems like a good approach!
I think we should deprecate generate-cask-api and generate-formula-api in favor of a single generate-package-api that does all of the api files except analytics, unless there's still a good reason to keep them separate (I don't remember what the initial thinking was)
Edit: one potential reason might be to keep generation time low, since the formula/cask jobs could run in parallel?
|
I pushed some changes, moving the generation to a new It's very much a work in progress, but hopefully you can see the vision. I left several to-do comments where I didn't finish working. I also haven't yet done any cask filtering, and realized I removed the filtering function you had written, apologies! (It's still in the original commit, I didn't force push. Just not the latest version) As always, feel free to modify/change direction/revert as desired. |
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.
Pull Request Overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 14 comments.
Files not reviewed (1)
- Library/Homebrew/sorbet/rbi/dsl/homebrew/dev_cmd/generate_package_api.rbi: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Pull Request Overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 16 comments.
Files not reviewed (1)
- Library/Homebrew/sorbet/rbi/dsl/homebrew/dev_cmd/generate_package_api.rbi: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
MikeMcQuaid
left a comment
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 @Rylan12! Think this approach makes sense. Some comments here may be repeating what we've already talked about or you've already decided but wanted to err on the side of overcommunicating than under!
| disable_replacement_cask: String, | ||
| disable_replacement_formula: String, | ||
| head_dependencies: T::Array[DependencyHash], | ||
| head_url: HeadURLHash, |
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.
@Rylan12 I think we could definitely inline any of these types that aren't in e.g. arrays
| caveats: T::Array[String], | ||
| conflicts_with: T::Array[String], | ||
| conflicts_with_reasons: T::Array[String], | ||
| deprecation_date: String, |
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.
Probably want to make all of these nilable so that they can be omitted from Internal API if unset.
| conflicts_with: T::Array[String], | ||
| conflicts_with_reasons: T::Array[String], |
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.
I think ideally this would less closely resemble the full API output format and instead just what the input format needs to be for Formulary.
Formulary should then consume a FormulaHash which can be provided from either the full API or the internal API. We should also then verify (either with Sorbet, runtime checks or RSpec tests) that FormulaHash is a 1:1 with the internal API data: we neither omit data that's needed nor include any data that's not needed.
9055235 to
4fee222
Compare
9a1f8d4 to
d672f8a
Compare
WIP but to give an idea so @Rylan12 can take a look