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.

Sign up for GitHub

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

Jump to bottom

Add generate-{cask,formula}-api commands #14762

Merged
merged 2 commits into from Feb 23, 2023
Merged

Add generate-{cask,formula}-api commands #14762

merged 2 commits into from Feb 23, 2023

Conversation

MikeMcQuaid
Copy link
Member

@MikeMcQuaid MikeMcQuaid commented Feb 22, 2023 •
edited

These replace the similar scripts in formulae.brew.sh.

Part of #14730.

See also Homebrew/formulae.brew.sh#776

@MikeMcQuaid
Add generate-{cask,formula}-api commands
8d02143 
These replace the similar scripts in formulae.brew.sh.

Part of #14730.
@BrewTestBot
Copy link
Member

Review period will end on 2023-02-23 at 15:35:40 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Feb 22, 2023
MikeMcQuaid added a commit to Homebrew/formulae.brew.sh that referenced this pull request Feb 22, 2023
@MikeMcQuaid
Move to generate-{cask,formula}-api commands
669e9f4 
Replace the scripts in this repository.

Depends on Homebrew/brew#14762

Fixes Homebrew/brew#14730
@MikeMcQuaid MikeMcQuaid mentioned this pull request Feb 22, 2023
Merged
@Bo98
Copy link
Member

Bo98 commented Feb 22, 2023

Will take a look this evening. Am I correct in this doesn't replace the Jekyll generation of some of the JSONs?

@MikeMcQuaid
Copy link
Member Author

Will take a look this evening. Am I correct in this doesn't replace the Jekyll generation of some of the JSONs?

@Bo98 No rush! Yeh, the e.g. formula.json is combined from all the individual formula files and same for cask.json.

Homebrew/formulae.brew.sh#776 contains the formulae.brew.sh parts so you can see what gets removed over there based on this.

MikeMcQuaid added a commit to Homebrew/formulae.brew.sh that referenced this pull request Feb 22, 2023
@MikeMcQuaid
Move to generate-{cask,formula}-api commands
4cdab43 
Replace the scripts in this repository.

Depends on Homebrew/brew#14762

Fixes Homebrew/brew#14730
Rylan12
Rylan12 approved these changes Feb 23, 2023
View reviewed changes
Copy link
Member

@Rylan12 Rylan12 left a comment •
edited

Choose a reason for hiding this comment

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

This looks great, thanks for working on this!

One non-blocking suggestion:

Library/Homebrew/dev-cmd/generate-cask-api.rb Show resolved Hide resolved
Library/Homebrew/dev-cmd/generate-formula-api.rb Show resolved Hide resolved
@Rylan12
Copy link
Member

Rylan12 commented Feb 23, 2023

Thoughts on also creating a brew generate-analytics-api command? Maybe if not here, in Homebrew/formula-analytics?

I think a lot of the logic in that command would be similar to what we're doing here

Bo98
Bo98 reviewed Feb 23, 2023
View reviewed changes
Library/Homebrew/dev-cmd/generate-cask-api.rb
Comment on lines +30 to +37
def html_template(title)
<<~EOS
---
title: #{title}
layout: cask
---
{{ content }}
EOS
Copy link
Member

@Bo98 Bo98 Feb 23, 2023 •
edited

Choose a reason for hiding this comment

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

The bit of Jekyll HTML template here feels really weird (particularly since this is not really the API itself) and am not a fan of it here. If it turns out to really be the only way to do it though then fair enough.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Bo98 Again: game to not do this. It's slightly nicer than a .gsub to make this a function, IMO.

Library/Homebrew/dev-cmd/generate-cask-api.rb
Comment on lines +23 to +28
JSON_TEMPLATE = <<~EOS
---
layout: cask_json
---
{{ content }}
EOS
Copy link
Member

@Bo98 Bo98 Feb 23, 2023 •
edited

Choose a reason for hiding this comment

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

I think eventually it would be nice to just bypass Jekyll for the JSON stuff tbh. Would make the JWS signing stuff an easy move here.

Library/Homebrew/dev-cmd/generate-cask-api.rb Outdated Show resolved Hide resolved
Library/Homebrew/dev-cmd/generate-formula-api.rb Outdated Show resolved Hide resolved
Library/Homebrew/dev-cmd/generate-formula-api.rb Outdated Show resolved Hide resolved
@MikeMcQuaid MikeMcQuaid added the critical Critical change which should be shipped as soon as possible. label Feb 23, 2023
@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Feb 23, 2023
@BrewTestBot
Copy link
Member

Review period skipped due to critical label.

@MikeMcQuaid
Copy link
Member Author

Thoughts on also creating a brew generate-analytics-api command? Maybe if not here, in Homebrew/formula-analytics?

Yeh, having it in Homebrew/formula-analytics would make sense to me. I didn't move it because they don't rely on Homebrew/brew internals and these commands did/do.

@MikeMcQuaid @Rylan12 @Bo98
Apply suggestions from code review
9c9213a 
Co-authored-by: Rylan Polster <rslpolster@gmail.com>
Co-authored-by: Bo Anderson <mail@boanderson.me>
@MikeMcQuaid
Copy link
Member Author

Marking as critical to get this merged and used in formulae.brew.sh now it's got a ✅.

@MikeMcQuaid MikeMcQuaid merged commit f6d1fa6 into Homebrew:master Feb 23, 2023
@MikeMcQuaid MikeMcQuaid deleted the generate_api branch February 23, 2023 09:02
@github-actions github-actions bot added the outdated PR was locked due to age label Mar 26, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Bo98 Bo98 Bo98 left review comments

@BrewTestBot BrewTestBot BrewTestBot approved these changes

@Rylan12 Rylan12 Rylan12 approved these changes

Assignees
No one assigned
Labels
critical Critical change which should be shipped as soon as possible. outdated PR was locked due to age
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@MikeMcQuaid @BrewTestBot @Bo98 @Rylan12