dev-cmd/edit: suggest tapping core repositories if untapped #15740

EricFromCanada · 2023-07-23T19:06:44Z

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?

Addresses the issue brought up in #15664 by suggesting homebrew/core or homebrew/cask be tapped when attempting to edit a formula or cask and either repo is untapped. Also expands edit's handling of tap arguments and fixes a few other related bugs.

Library/Homebrew/dev-cmd/edit.rb

Library/Homebrew/exceptions.rb

Library/Homebrew/dev-cmd/edit.rb

EricFromCanada · 2023-08-01T01:11:31Z

Library/Homebrew/cask/cask_loader.rb

@@ -244,7 +244,7 @@ def self.can_load?(ref)

      def initialize(token, from_json: nil)
        @token = token.sub(%r{^homebrew/(?:homebrew-)?cask/}i, "")
-        @path = CaskLoader.default_path(token)
+        @path = CaskLoader.default_path(@token)


Fixes an issue where args in the form "user/repo/name" wouldn't be handled properly.

EricFromCanada · 2023-08-01T01:13:06Z

Library/Homebrew/dev-cmd/edit.rb

@@ -24,7 +24,7 @@ def edit_args

      conflicts "--formula", "--cask"

-      named_args [:formula, :cask], without_api: true
+      named_args [:formula, :cask, :tap], without_api: true


This command hasn't advertised that it handles tap arguments too, until now.

EricFromCanada · 2023-08-01T01:19:03Z

Library/Homebrew/dev-cmd/edit.rb

      args.named.to_paths.select do |path|
-        next path if path.exist?
+        core_formula_path = path.fnmatch?("**/homebrew-core/Formula/*.rb", File::FNM_DOTMATCH)
+        core_cask_path = path.fnmatch?("**/homebrew-cask/Casks/*.rb", File::FNM_DOTMATCH)


File::FNM_DOTMATCH allows relative path arguments to be handled correctly.

We should modify this so it can handle sharded Formula/Cask directories.

Also, I wonder if it would be simpler/more robust to use #realpath/#ascend and/or GitRepository to find the repository root instead of with #fnmatch?, as this seems a bit fragile.

We should modify this so it can handle sharded Formula/Cask directories.

Agreed. Formula/**/*.rb should cover it. Ideally some of this logic would be in the Tap class.

**/homebrew-core/Formula/**.rb is what you want.

brew(main):020:0> Pathname("/usr/local/Homebrew/Library/Taps/homebrew/homebrew-cask/Casks/firefox.rb").fnmatch?("**/homebrew-cask/Casks/**.rb", File::FNM_DOTMATCH) => true brew(main):021:0> Pathname("/usr/local/Homebrew/Library/Taps/homebrew/homebrew-cask/Casks/f/firefox.rb").fnmatch?("**/homebrew-cask/Casks/**.rb", File::FNM_DOTMATCH) => true

Exactly, thanks.

EricFromCanada · 2023-08-01T01:19:24Z

Library/Homebrew/formulary.rb

@@ -1004,7 +1004,7 @@ def self.tap_paths(name, taps = Tap)
  end

  def self.find_formula_in_tap(name, tap)
-    filename = "#{name}.rb"
+    filename = "#{name}#{".rb" unless name.end_with?(".rb")}"


Fixes an issue where ".rb" would be unnecessarily added to a path argument that had it already.

Suggested change

filename = "#{name}#{".rb" unless name.end_with?(".rb")}"

filename = name

filename << ".rb" unless filename.end_with?(".rb")

Had to use name.dup otherwise tests like brew audit --skip-style --except=version openssl@3 would fail.

Library/Homebrew/cli/named_args.rb

carlocab · 2023-08-01T02:56:41Z

Library/Homebrew/dev-cmd/edit.rb

      args.named.to_paths.select do |path|
-        next path if path.exist?
+        core_formula_path = path.fnmatch?("**/homebrew-core/Formula/*.rb", File::FNM_DOTMATCH)
+        core_cask_path = path.fnmatch?("**/homebrew-cask/Casks/*.rb", File::FNM_DOTMATCH)


We should modify this so it can handle sharded Formula/Cask directories.

Also, I wonder if it would be simpler/more robust to use #realpath/#ascend and/or GitRepository to find the repository root instead of with #fnmatch?, as this seems a bit fragile.

Library/Homebrew/dev-cmd/edit.rb

carlocab · 2023-08-01T03:00:04Z

Library/Homebrew/dev-cmd/edit.rb

-          EOS
+        name = path.basename(".rb").to_s
+
+        if (tap_match = Regexp.new(HOMEBREW_TAP_DIR_REGEX.source + /$/.source).match(path.to_s))


Identifying the tap from a path seems like something we can extract to a method somewhere.

Any location in particular?

MikeMcQuaid

Thanks @EricFromCanada! Has been a lot of back and forth and this looks much better so would suggest any remaining/following comments get addressed in a follow-up PR.

apainintheneck reviewed Jul 24, 2023

View reviewed changes

Library/Homebrew/dev-cmd/edit.rb Outdated Show resolved Hide resolved

Library/Homebrew/dev-cmd/edit.rb Outdated Show resolved Hide resolved

Library/Homebrew/dev-cmd/edit.rb Outdated Show resolved Hide resolved

apainintheneck added the install from api Relates to API installs label Jul 24, 2023

EricFromCanada force-pushed the tap-suggestion branch 5 times, most recently from 6521a59 to 56a6562 Compare July 25, 2023 00:16

apainintheneck reviewed Jul 25, 2023

View reviewed changes

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

Library/Homebrew/dev-cmd/edit.rb Outdated Show resolved Hide resolved

Library/Homebrew/dev-cmd/edit.rb Outdated Show resolved Hide resolved

EricFromCanada force-pushed the tap-suggestion branch 3 times, most recently from 8cd4b31 to 2e77267 Compare July 29, 2023 02:19

MikeMcQuaid reviewed Jul 31, 2023

View reviewed changes

Library/Homebrew/dev-cmd/edit.rb Outdated Show resolved Hide resolved

EricFromCanada force-pushed the tap-suggestion branch 3 times, most recently from 78172b6 to 1306ac4 Compare August 1, 2023 00:57

EricFromCanada commented Aug 1, 2023

View reviewed changes

carlocab reviewed Aug 1, 2023

View reviewed changes

dev-cmd/edit: suggest tapping core repositories

b3ecd91

EricFromCanada force-pushed the tap-suggestion branch from 1306ac4 to b3ecd91 Compare August 1, 2023 17:39

MikeMcQuaid approved these changes Aug 2, 2023

View reviewed changes

MikeMcQuaid merged commit d2863d0 into Homebrew:master Aug 2, 2023
25 checks passed

EricFromCanada deleted the tap-suggestion branch August 2, 2023 12:21

carlocab mentioned this pull request Aug 7, 2023

a*: move to a/ subdirectory. Homebrew/homebrew-core#138921

Merged

github-actions bot added the outdated PR was locked due to age label Sep 2, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dev-cmd/edit: suggest tapping core repositories if untapped #15740

dev-cmd/edit: suggest tapping core repositories if untapped #15740

EricFromCanada commented Jul 23, 2023 •

edited

Loading

EricFromCanada Aug 1, 2023

EricFromCanada Aug 1, 2023

EricFromCanada Aug 1, 2023

carlocab Aug 1, 2023

MikeMcQuaid Aug 1, 2023

EricFromCanada Aug 1, 2023

MikeMcQuaid Aug 2, 2023

EricFromCanada Aug 1, 2023

carlocab Aug 1, 2023

EricFromCanada Aug 1, 2023

carlocab Aug 1, 2023

carlocab Aug 1, 2023

EricFromCanada Aug 1, 2023

MikeMcQuaid left a comment

	filename = "#{name}#{".rb" unless name.end_with?(".rb")}"
	filename = name
	filename << ".rb" unless filename.end_with?(".rb")

dev-cmd/edit: suggest tapping core repositories if untapped #15740

dev-cmd/edit: suggest tapping core repositories if untapped #15740

Conversation

EricFromCanada commented Jul 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid left a comment

Choose a reason for hiding this comment

EricFromCanada commented Jul 23, 2023 •

edited

Loading