Skip to content

Refactor FormulaCreator args and call parse_url automatically#20116

Merged
MikeMcQuaid merged 3 commits into
Homebrew:masterfrom
abitrolly:create-tests
Jun 17, 2025
Merged

Refactor FormulaCreator args and call parse_url automatically#20116
MikeMcQuaid merged 3 commits into
Homebrew:masterfrom
abitrolly:create-tests

Conversation

@abitrolly

@abitrolly abitrolly commented Jun 15, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor
  • 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 PR makes all FormulaCreator params, except URL optional, to make test calls less verbose.

Calls parse_url in constructor to ensure that supplied URL is automatically parsed (as discussed in https://github.com/Homebrew/brew/pull/20025/files#r2123010882).

The change includes fix from #20115 that restores tests run on Linux.

MikeMcQuaid
MikeMcQuaid reviewed Jun 16, 2025
View reviewed changes

@MikeMcQuaid MikeMcQuaid left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for splitting this out. A few nits but looks good so far.

Comment thread Library/Homebrew/formula_creator.rb
Comment thread Library/Homebrew/test/spec_helper.rb
MikeMcQuaid
MikeMcQuaid reviewed Jun 16, 2025
View reviewed changes
Comment thread Library/Homebrew/formula_creator.rb
Comment thread Library/Homebrew/test/formula_creator_spec.rb Outdated
Comment thread Library/Homebrew/test/formula_creator_spec.rb Outdated
MikeMcQuaid
MikeMcQuaid reviewed Jun 16, 2025
View reviewed changes
Comment thread Library/Homebrew/test/formula_creator_spec.rb Outdated
Comment thread Library/Homebrew/test/formula_creator_spec.rb Outdated
Comment thread Library/Homebrew/test/formula_creator_spec.rb Outdated
Comment thread Library/Homebrew/formula_creator.rb Outdated
Comment thread Library/Homebrew/formula_creator.rb Outdated
Comment thread Library/Homebrew/formula_creator.rb Outdated
Comment thread Library/Homebrew/formula_creator.rb
Comment thread Library/Homebrew/formula_creator.rb Outdated
Comment thread Library/Homebrew/formula_creator.rb Outdated
@abitrolly abitrolly force-pushed the create-tests branch 3 times, most recently from b4539e5 to 4c8a0df Compare June 16, 2025 18:07
MikeMcQuaid
MikeMcQuaid reviewed Jun 17, 2025
View reviewed changes
Comment thread Library/Homebrew/test/formula_creator_spec.rb Outdated
@abitrolly

abitrolly commented Jun 17, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@MikeMcQuaid addressed all comments. What to do with brew typecheck failure? Should I revert types?

EDIT: Nevermind. This is ready for review.

@abitrolly @MikeMcQuaid
Refactor FormulaCreator args and call parse_url automatically
85d48da 
Co-authored-by: Mike McQuaid <mike@mikemcquaid.com>
@MikeMcQuaid

MikeMcQuaid commented Jun 17, 2025

Copy link
Copy Markdown
Member

@abitrolly Please stop pushing to this PR. I'm going to fix it.

@MikeMcQuaid

MikeMcQuaid commented Jun 17, 2025

Copy link
Copy Markdown
Member

@abitrolly Please test the latest version here. I've completed the typechecking and refactoring.

abitrolly
abitrolly commented Jun 17, 2025
View reviewed changes

@abitrolly abitrolly left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Now I see what you mean by combining parsing logic into constructor. The assignment of instance variables became more clear. Looks good.

I kind of disagree with the way test data is processed, but that's not a blocker. Just a personal preference from Go/Python side.

Comment thread Library/Homebrew/dev-cmd/create.rb Outdated
Comment thread Library/Homebrew/test/formula_creator_spec.rb
Comment thread Library/Homebrew/test/formula_creator_spec.rb
Comment thread Library/Homebrew/formula_creator.rb
MikeMcQuaid
MikeMcQuaid reviewed Jun 17, 2025
View reviewed changes

@MikeMcQuaid MikeMcQuaid left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@abitrolly Can you clarify whether you tested this or not? Please can we move this to a conclusion? For refactoring with no functionality change this PR has wasted a lot of time and I am losing my patience with the amount of back and forth required here.

Comment thread Library/Homebrew/dev-cmd/create.rb Outdated
Comment thread Library/Homebrew/formula_creator.rb
Comment thread Library/Homebrew/test/formula_creator_spec.rb
Comment thread Library/Homebrew/test/formula_creator_spec.rb
@MikeMcQuaid @abitrolly
dev-cmd/create: avoid duplicate presence check.
1f29f51 
Co-authored-by: Anatoli Babenia <anatoli@rainforce.org>
@abitrolly

abitrolly commented Jun 17, 2025

Copy link
Copy Markdown
Contributor Author

@MikeMcQuaid I've tested it with brew tests and manually run with and without new name. Works as expected.

brew create https://github.com/buildpacks/pack/archive/refs/tags/v0.37.0.tar.gz -d

@abitrolly

abitrolly commented Jun 17, 2025

Copy link
Copy Markdown
Contributor Author

The conclusion is that it is ready long ago. )

@MikeMcQuaid MikeMcQuaid self-requested a review June 17, 2025 14:18
@MikeMcQuaid MikeMcQuaid added this pull request to the merge queue Jun 17, 2025
@MikeMcQuaid

MikeMcQuaid commented Jun 17, 2025

Copy link
Copy Markdown
Member

Thanks @abitrolly.

Merged via the queue into Homebrew:master with commit 3c30845 Jun 17, 2025
33 checks passed
@abitrolly abitrolly deleted the create-tests branch June 17, 2025 14:46
@abitrolly abitrolly mentioned this pull request Jun 18, 2025
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MikeMcQuaid MikeMcQuaid MikeMcQuaid 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

@abitrolly @MikeMcQuaid