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

Display collected caveats at end of install or upgrade #4361

Merged
merged 2 commits into from
Jul 13, 2018
Merged
Show file tree
Hide file tree
Changes from all 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
1 change: 1 addition & 0 deletions Library/Homebrew/cmd/install.rb
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,7 @@ def install
Migrator.migrate_if_needed(f)
install_formula(f)
end
Homebrew.messages.display_messages
rescue FormulaUnreadableError, FormulaClassUnavailableError,
TapFormulaUnreadableError, TapFormulaClassUnavailableError => e
# Need to rescue before `FormulaUnavailableError` (superclass of this)
Expand Down
2 changes: 2 additions & 0 deletions Library/Homebrew/cmd/reinstall.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

require "formula_installer"
require "development_tools"
require "messages"

module Homebrew
module_function
Expand All @@ -18,6 +19,7 @@ def reinstall
Migrator.migrate_if_needed(f)
reinstall_formula(f)
end
Homebrew.messages.display_messages
end

def reinstall_formula(f)
Expand Down
2 changes: 2 additions & 0 deletions Library/Homebrew/cmd/upgrade.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
require "formula_installer"
require "cleanup"
require "development_tools"
require "messages"

module Homebrew
module_function
Expand Down Expand Up @@ -102,6 +103,7 @@ def upgrade
onoe "#{f}: #{e}"
end
end
Homebrew.messages.display_messages
end

def upgrade_formula(f)
Expand Down
3 changes: 3 additions & 0 deletions Library/Homebrew/formula_installer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
require "cache_store"
require "linkage_checker"
require "install"
require "messages"

class FormulaInstaller
include FormulaCellarChecks
Expand Down Expand Up @@ -348,6 +349,7 @@ def install
build_bottle_postinstall if build_bottle?

opoo "Nothing was installed to #{formula.prefix}" unless formula.installed?
Homebrew.messages.formula_installed(formula)
end

def check_conflicts
Expand Down Expand Up @@ -604,6 +606,7 @@ def caveats
return if caveats.empty?
@show_summary_heading = true
ohai "Caveats", caveats.to_s
Homebrew.messages.record_caveats(formula, caveats)
end

def finish
Expand Down
5 changes: 5 additions & 0 deletions Library/Homebrew/global.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
require "pathname"
require "English"
require "ostruct"
require "messages"

require "pp"
require "extend/ARGV"
Expand Down Expand Up @@ -49,6 +50,10 @@ def args
@args ||= OpenStruct.new
end

def messages
@messages ||= Messages.new
end

def raise_deprecation_exceptions?
@raise_deprecation_exceptions == true
end
Expand Down
31 changes: 31 additions & 0 deletions Library/Homebrew/messages.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
# A Messages object collects messages that may need to be displayed together
# at the end of a multi-step `brew` command run
class Messages
Copy link
Member

Choose a reason for hiding this comment

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

Could we get some basic unit tests for this class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Give me a little while...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you want to see the Messages class tested in isolation in-process, or an example of shelling out to a brew command that produces collected caveats?

Copy link
Member

Choose a reason for hiding this comment

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

@apjanke Tested in process/unit tests please, thanks!

Copy link
Member

Choose a reason for hiding this comment

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

@apjanke Gentle ping on some unit tests, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added unit tests.

attr_reader :caveats, :formula_count

def initialize
@caveats = []
@formula_count = 0
end

def record_caveats(f, caveats)
@caveats.push(formula: f.name, caveats: caveats)
end

def formula_installed(_f)
@formula_count += 1
end

def display_messages
display_caveats
end

def display_caveats
return if @formula_count <= 1
return if @caveats.empty?
oh1 "Caveats"
@caveats.each do |c|
ohai c[:formula], c[:caveats]
end
end
end
36 changes: 36 additions & 0 deletions Library/Homebrew/test/messages_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
require "messages"
require "spec_helper"

describe Messages do
before do
@m = Messages.new
f_foo = formula("foo") do
url "http://example.com/foo-0.1.tgz"
end
f_bar = formula("bar") do
url "http://example.com/bar-0.1.tgz"
end
f_baz = formula("baz") do
url "http://example.com/baz-0.1.tgz"
end
@m.formula_installed(f_foo)
@m.record_caveats(f_foo, "Zsh completions were installed")
@m.formula_installed(f_bar)
@m.record_caveats(f_bar, "Keg-only formula")
@m.formula_installed(f_baz)
@m.record_caveats(f_baz, "A valid GOPATH is required to use the go command")
end

it "has the right installed-formula count" do
expect(@m.formula_count).to equal(3)
end

it "has recorded caveats" do
expect(@m.caveats).to_not be_empty
end

it "maintained the order of recorded caveats" do
caveats_formula_order = @m.caveats.map { |x| x[:formula] }
expect(caveats_formula_order).to eq(["foo", "bar", "baz"])
end
end