Refactor `Formulary::loader_for`. #16623

reitermarkus · 2024-02-08T18:25:37Z

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?

Use same pattern as in CaskLoader::for.

apainintheneck · 2024-02-10T04:59:54Z

At a glance, this seems much nicer from a maintainability and readability perspective in. I can't help but think though that we should avoid further changes here until we have some idea why the previous refactor, #16628, caused some problems for users even if it ended up being a problem with a third-party tap.

reitermarkus · 2024-02-10T11:46:05Z

I will have a look at #16628 to see what causes it.

Library/Homebrew/formulary.rb

MikeMcQuaid

Looks ok so far. Mildly concerned about potential for regressions.

Library/Homebrew/formula.rb

Library/Homebrew/tap_constants.rb

Bo98 · 2024-02-12T17:37:03Z

(Will do a full review tomorrow)

Bo98

Apologies for the delay - was a lot to check over.

Library/Homebrew/diagnostic.rb

Bo98 · 2024-02-14T15:46:10Z

Library/Homebrew/test/formulary_spec.rb

@@ -111,7 +111,7 @@ class Wrong#{described_class.class_s(formula_name)} < Formula
      it "raises an error" do
        expect do
          described_class.factory(formula_name)
-        end.to raise_error(FormulaClassUnavailableError)
+        end.to raise_error(TapFormulaClassUnavailableError)


Should this really be going through FromTapLoader? I'm cautious whether this behaviour change is a regression.

It is loaded from a tap, so the error is correct. I think the only way to get a FormulaClassUnavailableError now is loading from a bottle, URL or path outside of a tap.

Ok so the change is CoreTap now throws this whereas it previously didn't?

Any tap will throw this when using only the short name to load a formula.

Library/Homebrew/formulary.rb

Bo98 · 2024-02-14T15:55:03Z

Library/Homebrew/formulary.rb

+
+      ref = "#{CoreTap.instance}/#{name}"
+
+      name, tap, type = Formulary.tap_formula_name_type(ref, warn: warn)


What happens if this hits a migration to Homebrew/cask (i.e. tap.core_tap? is no longer true)?

It seems like tap migrations are not handled at all when using the API, so it is never hit. See the early return above, which only checks names/aliases/renames, not tap migrations. This was also the case before this change.

Library/Homebrew/formulary.rb

Bo98 · 2024-02-14T16:09:35Z

Library/Homebrew/formulary.rb


-    def find_formula_from_name(name, tap)
-      Formulary.find_formula_in_tap(name, tap)
+        raise TapFormulaAmbiguityError.new(name, loaders.map(&:tap))


Suggested change

raise TapFormulaAmbiguityError.new(name, loaders.map(&:tap))

if type == :rename

raise TapFormulaWithOldnameAmbiguityError.new(???)

else

raise TapFormulaAmbiguityError.new(name, loaders.map(&:tap))

end

Or alternatively burn TapFormulaWithOldnameAmbiguityError entirely, though this PR is already quite large.

Looking at the old logic, it seems like this expects the same old name to occur in multiple taps, rather than a single rename in the default tap conflicting with other taps' formula names. This seems weirdly specific, so I think removing TapFormulaWithOldnameAmbiguityError makes more sense.

Bo98 · 2024-02-14T22:48:42Z

Library/Homebrew/diagnostic.rb

+          loadable = [
+            Formulary::FromAPILoader,
+            Formulary::FromNameLoader,
+          ].any? do |loader_class|


Does FromDefaultNameLoader need to be added here now (assuming HOMEBREW_NO_INSTALL_FROM_API=1) or does this still work as expected?

It should work without it because of the warn: false, but doesn't hurt to add it here.

Bo98

One last question above, but I'm happy otherwise. Thanks!

MikeMcQuaid

Seems fine when/if @Bo98 is fine too, thanks!

reitermarkus requested review from Bo98 and apainintheneck February 8, 2024 18:25

reitermarkus force-pushed the formulary-loader-for branch 10 times, most recently from 059ab7f to f6f6312 Compare February 9, 2024 16:59

reitermarkus force-pushed the formulary-loader-for branch 2 times, most recently from 1e17dee to ebe652f Compare February 10, 2024 14:21

apainintheneck reviewed Feb 11, 2024

View reviewed changes

Library/Homebrew/formulary.rb Outdated Show resolved Hide resolved

Library/Homebrew/formulary.rb Outdated Show resolved Hide resolved

apainintheneck mentioned this pull request Feb 11, 2024

Fix support of formulae aliases in taps #16637

Merged

7 tasks

reitermarkus force-pushed the formulary-loader-for branch from ebe652f to f4b9658 Compare February 12, 2024 06:27

reitermarkus requested a review from apainintheneck February 12, 2024 07:08

MikeMcQuaid reviewed Feb 12, 2024

View reviewed changes

Library/Homebrew/formula.rb Show resolved Hide resolved

Bo98 reviewed Feb 12, 2024

View reviewed changes

Library/Homebrew/tap_constants.rb Outdated Show resolved Hide resolved

reitermarkus force-pushed the formulary-loader-for branch 2 times, most recently from d9e0661 to 170723c Compare February 13, 2024 00:14

Bo98 reviewed Feb 14, 2024

View reviewed changes

reitermarkus added 4 commits February 14, 2024 20:28

Refactor Formulary::loader_for.

4641690

Fix code style.

d70f719

Revert diagnostic change.

ecee4ae

Remove Formulary::tap_paths again.

0f4d912

reitermarkus added 10 commits February 14, 2024 20:28

Remove redundant debug output.

0626f62

Fix loading versioned casks/formulae from API.

61a283e

Avoid T.cast.

ba29328

Simplify conditional.

22bb157

Simplify test.

c5a877d

Remove unnecessary if.

562d529

Always prefer the default tap.

eb48d6b

Fix tap constants.

726aeca

Add separate loader for default tap.

9056613

Handle TapFormulaAmbiguityError in diagnostics.

ce60048

reitermarkus force-pushed the formulary-loader-for branch from 170723c to ce60048 Compare February 14, 2024 19:28

reitermarkus added 4 commits February 14, 2024 22:11

Fix type signature.

f4ac299

Fix code style.

177e916

Remove unused TapFormulaWithOldnameAmbiguityError.

b717f5d

Fix FromDefaultNameLoader.

fe1f6c9

reitermarkus force-pushed the formulary-loader-for branch from db8bdc6 to fe1f6c9 Compare February 14, 2024 22:09

reitermarkus requested a review from Bo98 February 14, 2024 22:10

Bo98 reviewed Feb 14, 2024

View reviewed changes

Bo98 approved these changes Feb 14, 2024

View reviewed changes

Use FromDefaultNameLoader for diagnostics.

efa6130

MikeMcQuaid approved these changes Feb 15, 2024

View reviewed changes

reitermarkus merged commit ebd30fb into Homebrew:master Feb 15, 2024
31 checks passed

reitermarkus deleted the formulary-loader-for branch February 15, 2024 17:09

MikeMcQuaid mentioned this pull request Feb 16, 2024

Revert "Refactor Formulary::loader_for." #16684

Merged

github-actions bot added the outdated PR was locked due to age label Mar 18, 2024

github-actions bot locked as resolved and limited conversation to collaborators Mar 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `Formulary::loader_for`. #16623

Refactor `Formulary::loader_for`. #16623

reitermarkus commented Feb 8, 2024 •

edited

apainintheneck commented Feb 10, 2024

reitermarkus commented Feb 10, 2024

MikeMcQuaid left a comment

Bo98 commented Feb 12, 2024

Bo98 left a comment

Bo98 Feb 14, 2024 •

edited

reitermarkus Feb 14, 2024 •

edited

Bo98 Feb 14, 2024

reitermarkus Feb 14, 2024

Bo98 Feb 14, 2024 •

edited

reitermarkus Feb 14, 2024

Bo98 Feb 14, 2024

reitermarkus Feb 14, 2024

Bo98 Feb 14, 2024 •

edited

reitermarkus Feb 15, 2024

Bo98 left a comment

MikeMcQuaid left a comment


		ref = "#{CoreTap.instance}/#{name}"

		name, tap, type = Formulary.tap_formula_name_type(ref, warn: warn)

Refactor Formulary::loader_for. #16623

Refactor Formulary::loader_for. #16623

Conversation

reitermarkus commented Feb 8, 2024 • edited

apainintheneck commented Feb 10, 2024

reitermarkus commented Feb 10, 2024

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Bo98 commented Feb 12, 2024

Bo98 left a comment

Choose a reason for hiding this comment

Bo98 Feb 14, 2024 • edited

Choose a reason for hiding this comment

reitermarkus Feb 14, 2024 • edited

Choose a reason for hiding this comment

Bo98 Feb 14, 2024

Choose a reason for hiding this comment

reitermarkus Feb 14, 2024

Choose a reason for hiding this comment

Bo98 Feb 14, 2024 • edited

Choose a reason for hiding this comment

reitermarkus Feb 14, 2024

Choose a reason for hiding this comment

Bo98 Feb 14, 2024

Choose a reason for hiding this comment

reitermarkus Feb 14, 2024

Choose a reason for hiding this comment

Bo98 Feb 14, 2024 • edited

Choose a reason for hiding this comment

reitermarkus Feb 15, 2024

Choose a reason for hiding this comment

Bo98 left a comment

Choose a reason for hiding this comment

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Refactor `Formulary::loader_for`. #16623

Refactor `Formulary::loader_for`. #16623

reitermarkus commented Feb 8, 2024 •

edited

Bo98 Feb 14, 2024 •

edited

reitermarkus Feb 14, 2024 •

edited

Bo98 Feb 14, 2024 •

edited

Bo98 Feb 14, 2024 •

edited