Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Load internal json v3 #16638
Load internal json v3 #16638
Changes from all commits
d64de40
670e218
cefd327
e9a05d8
200fe2a
bfe5e43
f1f2e24
99c101b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 77 in Library/Homebrew/dev-cmd/generate-formula-api.rb
Library/Homebrew/dev-cmd/generate-formula-api.rb#L77
Check warning on line 395 in Library/Homebrew/formulary.rb
Library/Homebrew/formulary.rb#L395
Check warning on line 400 in Library/Homebrew/formulary.rb
Library/Homebrew/formulary.rb#L400
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.
Sorry to increase up scope here potentially (and can be in another PR) but: this API is fairly awful due to backwards compatibility concerns we don't have for v3 so would be nice to address. CC @Bo98 for thoughts on what that should look like.
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.
Yeah we were going to cycle back and clean this up. I can open a PR with suggestions (probably easier after merging this PR though)
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.
Cool: given that: fine to merge with this as-is in this PR 👍🏻. A TODO or something might be nice though?
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.
Can a TODO be added here please.
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 left a note in the original issue thread: #16410 (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.
It seems weird to leave a comment when any potential changes are exploratory. We haven't decided on the form that we want yet.
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.
@apainintheneck It's a hard
TODO
because we're not going to ship the internal JSON v3 without changing this. I'm fine on not blocking the PR on this stuff but I would really like something beyond a comment in another issue to track this in the actual code.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.
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'm fine with the comment but this is news to me. I didn't know that this was that important either way.
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.
Apologies for my unclear communication! I'm trying to get the balance right between being overly pedantic/demotivating and blocking individual PRs vs. making clear what's required before we ship to users. Great work getting this shipped, thanks @apainintheneck ❤️
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.
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.
Yeah, I can probably compact or avoid adding that field in the first place.
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.
Never mind, this only exists in the public facing JSON and should already be omitted from the internal one. I'm just using the public JSON here for testing because it exercises more parts of the formula code.
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, I think it'd be preferable to use the internal JSON and to get full coverage of the formula code through the input JSON.
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.
Sorry, I don't know what you mean by this. How can we get full coverage of the formula code through the input JSON? What input JSON are we referring to here?
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'm saying it seems very weird to be to use the non-internal public JSON for testing the internal JSON format?
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.
It's not that weird. It essentially mimics
brew readall
.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.
@apainintheneck Ok but aren't these JSON formats going to diverge significantly?
I think I might just need an ELI5 treatment here, sorry.
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 basically a fancy
to_hash
test. We could do it as a series of checks instead, though would be multipleexpects
in a single check so might be long: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.
#to_hash
is a bit better than doing a bunch of individual checks since it will end up calling more methods internally to fill out the fields we're not checking.The goal here is not to validate the
#to_hash
method but instead to be reasonably confident that the formula loads without failing.#to_hash
is just an easy way to take a peek inside the formula instance and run some arbitrary methods along the way. I'm open to alternatives that would be clearer, briefer or provide more assurance of correctness. The current test is not comprehensive at all.#to_api_hash
is already being exercised by a different test where we generate the JSON API for the core formula tap.