Skip to content

Ensure we require "formula" before doing ensure_formula_installed! #20358

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
Merged
merged 2 commits into from
Aug 1, 2025
Merged

Ensure we require "formula" before doing ensure_formula_installed! #20358

merged 2 commits into from
Aug 1, 2025

Conversation

carlocab
Copy link
Member

@carlocab carlocab commented Aug 1, 2025

  • 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 needed to avoid Sorbet runtime errors. See #20352.

@carlocab carlocab requested a review from Bo98 August 1, 2025 18:42
carlocab
carlocab commented Aug 1, 2025
View reviewed changes
@carlocab carlocab force-pushed the require-formula-ensure-formula-installed branch from 31cbc10 to 1a89dc9 Compare August 1, 2025 18:44
@carlocab
Ensure we require "formula" before doing ensure_formula_installed!
cc5df09 
This is needed to avoid Sorbet runtime errors. See #20352.
@carlocab carlocab force-pushed the require-formula-ensure-formula-installed branch from 1a89dc9 to cc5df09 Compare August 1, 2025 18:46
Bo98
Bo98 approved these changes Aug 1, 2025
View reviewed changes
@carlocab
style: remove unused method
3d4df6f 
There is no `rubocop` formula, so this method cannot possibly be used
without erroring out.
@Bo98
Copy link
Member

Bo98 commented Aug 1, 2025

This does make me think however that maybe we should just move this to be Formula[name].ensure_installed! at some point.

Base automatically changed from more-extend-kernel-sigs to main August 1, 2025 19:08
@carlocab
Copy link
Member Author

carlocab commented Aug 1, 2025

This does make me think however that maybe we should just move this to be Formula[name].ensure_installed! at some point.

Yea, that seems better. Probably worth a deprecation cycle, though.

@carlocab carlocab enabled auto-merge August 1, 2025 19:14
@carlocab carlocab added this pull request to the merge queue Aug 1, 2025
carlocab added a commit that referenced this pull request Aug 1, 2025
@carlocab
Replace ensure_formula_installed! with Formula#ensure_installed!
0cd2266 
`ensure_formula_installed!` requires the `Formula` class to be loaded
before being called to work properly.

Let's guarantee that instead by implementing it as an instance method of
the `Formula` class.

See discussion at #20358.
Merged via the queue into main with commit 9dc1112 Aug 1, 2025
36 checks passed
@carlocab carlocab deleted the require-formula-ensure-formula-installed branch August 1, 2025 19:40
carlocab added a commit that referenced this pull request Aug 1, 2025
@carlocab
Replace ensure_formula_installed! with Formula#ensure_installed!
fb35add 
`ensure_formula_installed!` requires the `Formula` class to be loaded
before being called to work properly.

Let's guarantee that instead by implementing it as an instance method of
the `Formula` class.

See discussion at #20358.
@carlocab carlocab mentioned this pull request Aug 1, 2025
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Bo98 Bo98 Bo98 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@carlocab @Bo98