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.

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

dev-cmd/edit: Move path specific functions to Pathname #16029

Merged
merged 16 commits into from
Sep 27, 2023
Merged
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
123 changes: 75 additions & 48 deletions Library/Homebrew/dev-cmd/edit.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,32 @@
module Homebrew
module_function

module Refinements
abitrolly marked this conversation as resolved.
Show resolved Hide resolved
refine Pathname do
sig { returns(T::Boolean) }
def core_formula_path?
fnmatch?("**/homebrew-core/Formula/**.rb", File::FNM_DOTMATCH)
end

sig { returns(T::Boolean) }
def core_cask_path?
fnmatch?("**/homebrew-cask/Casks/**.rb", File::FNM_DOTMATCH)
end

sig { returns(T::Boolean) }
def core_formula_tap?
self == CoreTap.instance.path
end

sig { returns(T::Boolean) }
def core_cask_tap?
self == CoreCaskTap.instance.path
end
end
end

using Refinements
abitrolly marked this conversation as resolved.
Show resolved Hide resolved

sig { returns(CLI::Parser) }
def edit_args
Homebrew::CLI::Parser.new do
Expand All @@ -28,6 +54,40 @@ def edit_args
end
end

sig { params(path: Pathname, cask: T::Boolean).void }
def fail_with_message(path, cask)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def fail_with_message(path, cask)
def raise_unless_existing!(path, cask)

Copy link
Member

Choose a reason for hiding this comment

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

@abitrolly Still prefer this naming or at least something with raise and the ! to be more in keeping with typical Ruby behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed to raise_with_message!

name = path.basename(".rb").to_s

if (tap_match = Regexp.new("#{HOMEBREW_TAP_DIR_REGEX.source}$").match(path.to_s))
raise TapUnavailableError, CoreTap.instance.name if path.core_formula_tap?
raise TapUnavailableError, CoreCaskTap.instance.name if path.core_cask_tap?

raise TapUnavailableError, "#{tap_match[:user]}/#{tap_match[:repo]}"
elsif cask || path.core_cask_path?
if !CoreCaskTap.instance.installed? && Homebrew::API::Cask.all_casks.key?(name)
command = "brew tap --force #{CoreCaskTap.instance.name}"
action = "tap #{CoreCaskTap.instance.name}"
else
command = "brew create --cask --set-name #{name} $URL"
action = "create a new cask"
end
elsif path.core_formula_path? &&
!CoreTap.instance.installed? &&
Homebrew::API::Formula.all_formulae.key?(name)
command = "brew tap --force #{CoreTap.instance.name}"
action = "tap #{CoreTap.instance.name}"
else
command = "brew create --set-name #{name} $URL"
action = "create a new formula"
end

message = <<~EOS
#{name} doesn't exist on disk.
Run #{Formatter.identifier(command)} to #{action}!
EOS
raise UsageError, message
abitrolly marked this conversation as resolved.
Show resolved Hide resolved
end

sig { void }
def edit
args = edit_args.parse
Expand All @@ -50,56 +110,23 @@ def edit
[HOMEBREW_REPOSITORY]
end
else
edit_api_message_displayed = T.let(false, T::Boolean)
args.named.to_paths.select do |path|
core_formula_path = path.fnmatch?("**/homebrew-core/Formula/**.rb", File::FNM_DOTMATCH)
core_cask_path = path.fnmatch?("**/homebrew-cask/Casks/**.rb", File::FNM_DOTMATCH)
core_formula_tap = path == CoreTap.instance.path
core_cask_tap = path == CoreCaskTap.instance.path

if path.exist?
if (core_formula_path || core_cask_path || core_formula_tap || core_cask_tap) &&
!edit_api_message_displayed &&
!Homebrew::EnvConfig.no_install_from_api? &&
!Homebrew::EnvConfig.no_env_hints?
opoo <<~EOS
`brew install` ignores locally edited #{(core_cask_path || core_cask_tap) ? "casks" : "formulae"} if
`HOMEBREW_NO_INSTALL_FROM_API` is not set.
EOS
edit_api_message_displayed = true
end
next path
end
expanded_paths = args.named.to_paths
expanded_paths.each do |path|
fail_with_message(path, args.cask?) unless path.exist?
Copy link
Member

Choose a reason for hiding this comment

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

How about moving the unless path.exist? into raise_unless_existing ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can be done, but I would rather declare fail_with_message as T.noreturn to keep the branch flow in edit. So what is your decision?

Copy link
Member

Choose a reason for hiding this comment

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

You can still have it as void as long as the return value is not used by callers.

end

name = path.basename(".rb").to_s

if (tap_match = Regexp.new(HOMEBREW_TAP_DIR_REGEX.source + /$/.source).match(path.to_s))
raise TapUnavailableError, CoreTap.instance.name if core_formula_tap
raise TapUnavailableError, CoreCaskTap.instance.name if core_cask_tap

raise TapUnavailableError, "#{tap_match[:user]}/#{tap_match[:repo]}"
elsif args.cask? || core_cask_path
if !CoreCaskTap.instance.installed? && Homebrew::API::Cask.all_casks.key?(name)
command = "brew tap --force #{CoreCaskTap.instance.name}"
action = "tap #{CoreCaskTap.instance.name}"
else
command = "brew create --cask --set-name #{name} $URL"
action = "create a new cask"
end
elsif core_formula_path && !CoreTap.instance.installed? && Homebrew::API::Formula.all_formulae.key?(name)
command = "brew tap --force #{CoreTap.instance.name}"
action = "tap #{CoreTap.instance.name}"
else
command = "brew create --set-name #{name} $URL"
action = "create a new formula"
expanded_paths.each do |path|
if (path.core_formula_path? || path.core_cask_path? || path.core_formula_tap? || path.core_cask_tap?) &&
!Homebrew::EnvConfig.no_install_from_api? &&
!Homebrew::EnvConfig.no_env_hints?
opoo <<~EOS
`brew install` ignores locally edited #{(path.core_cask_path? || path.core_cask_tap?) ? "casks" : "formulae"} if
abitrolly marked this conversation as resolved.
Show resolved Hide resolved
HOMEBREW_NO_INSTALL_FROM_API is not set.
EOS
break
end

message = <<~EOS
#{name} doesn't exist on disk.
Run #{Formatter.identifier(command)} to #{action}!
EOS
raise UsageError, message
end.presence
end
expanded_paths
end

if args.print_path?
Expand Down
Loading