-
-
Notifications
You must be signed in to change notification settings - Fork 10.9k
Use FormulaStruct when loading formulae from the API
#21176
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
Conversation
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.
Looks great so far! Goal here would be to simplify the formulary logic as much as possible while still making it responsible for actually calling the relevant DSL methods.
Feel free to punt on the map comments etc.: that could be instead done in another, later PR/pass.
|
This is still in-progress, but here's a different approach that makes |
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.
Yeh, this looks really great!
a9e7907 to
3428b31
Compare
|
This is mostly there, but the service blocks don't quite work yet |
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.
This is looking so good!
|
Oh, another comment while I'm thinking about it: I removed a few lines that existed for backward compatibility with previous iterations of the api (e.g. I don't think we ever attempt to load old JSON data, so this seems like it should be okay. I think in the future: the only backward compatibility we should need here is between the latest release and now, right? |
Agreed. |
2ba81da to
c341fe8
Compare
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 introduces a new FormulaStruct class that serves as an intermediary data structure between the Homebrew JSON API and formula instantiation in Formulary. The refactoring extracts complex formula parsing logic from Formulary.load_formula_from_json! into a dedicated struct and helper method, improving code organization and maintainability. This abstraction also sets the groundwork for loading formulae from an internal API in the future.
Key changes:
- Introduces
Homebrew::API::FormulaStructwith typed fields and predicate methods for all formula properties - Adds
Homebrew::API::Formula.generate_formula_struct_hashto transform API JSON into struct-compatible format - Simplifies
Formulary.load_formula_from_json!by delegating to the struct for data access - Creates SPDX::LicenseExpression type alias for improved type clarity
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| Library/Homebrew/api/formula_struct.rb | New struct class defining typed fields for all formula properties with predicate methods and dependency/requirement parsing logic |
| Library/Homebrew/api/formula.rb | Adds generate_formula_struct_hash method to transform JSON API responses into FormulaStruct-compatible format |
| Library/Homebrew/formulary.rb | Refactored to use FormulaStruct for cleaner, more maintainable formula loading from API |
| Library/Homebrew/utils/spdx.rb | Introduces LicenseExpression type alias to replace verbose inline type definitions |
| Library/Homebrew/formula.rb | Updates type signatures to use new SPDX::LicenseExpression type alias |
| Library/Homebrew/dependency_collector.rb | Updates comment referencing moved API_SUPPORTED_REQUIREMENTS constant |
| Library/Homebrew/api.rb | Adds require for new formula_struct file |
| Library/Homebrew/test/formulary_spec.rb | Updates test data to include new required fields (specs, tap_git_head, expanded deprecation/disable fields) |
| Library/Homebrew/sorbet/tapioca/compilers/api_structs.rb | New Tapioca compiler for generating RBI signatures for FormulaStruct predicate methods |
| Library/Homebrew/sorbet/rbi/dsl/homebrew/api/formula_struct.rbi | Generated RBI file with predicate method signatures for FormulaStruct |
Files not reviewed (1)
- Library/Homebrew/sorbet/rbi/dsl/homebrew/api/formula_struct.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.
@Rylan12 Great work here. Happy for you to merge this whenever it's 🟢 and you're happy with it. Any changes/cleanup could be made in a follow-up.
c341fe8 to
2a5dc71
Compare
2a5dc71 to
472cb0a
Compare
This PR adds a new
FormulaStructto serve as an intermediary between the API and the formula loading inFormulary.Eventually, this will be able to load from the internal API as well.