From 232ec4679a808f0f3371e5326ff856a8a635334b Mon Sep 17 00:00:00 2001 From: Parker Moore Date: Fri, 18 Aug 2017 12:45:23 -0400 Subject: [PATCH] Allow `yield` to logger methods & bail early on no-op messages (#6315) Merge pull request 6315 --- lib/jekyll/log_adapter.rb | 64 ++++++++++++++++++++++++++++----------- test/helper.rb | 7 +++-- test/test_log_adapter.rb | 24 ++++++++++----- test/test_new_command.rb | 13 +++----- test/test_site.rb | 8 ++--- 5 files changed, 78 insertions(+), 38 deletions(-) diff --git a/lib/jekyll/log_adapter.rb b/lib/jekyll/log_adapter.rb index ec33b4d90d8..65b8f9deb7b 100644 --- a/lib/jekyll/log_adapter.rb +++ b/lib/jekyll/log_adapter.rb @@ -2,7 +2,7 @@ module Jekyll class LogAdapter - attr_reader :writer, :messages + attr_reader :writer, :messages, :level LOG_LEVELS = { :debug => ::Logger::DEBUG, @@ -30,6 +30,7 @@ def initialize(writer, level = :info) # Returns nothing def log_level=(level) writer.level = LOG_LEVELS.fetch(level) + @level = level end def adjust_verbosity(options = {}) @@ -48,8 +49,8 @@ def adjust_verbosity(options = {}) # message - the message detail # # Returns nothing - def debug(topic, message = nil) - writer.debug(message(topic, message)) + def debug(topic, message = nil, &block) + write(:debug, topic, message, &block) end # Public: Print a message @@ -58,8 +59,8 @@ def debug(topic, message = nil) # message - the message detail # # Returns nothing - def info(topic, message = nil) - writer.info(message(topic, message)) + def info(topic, message = nil, &block) + write(:info, topic, message, &block) end # Public: Print a message @@ -68,8 +69,8 @@ def info(topic, message = nil) # message - the message detail # # Returns nothing - def warn(topic, message = nil) - writer.warn(message(topic, message)) + def warn(topic, message = nil, &block) + write(:warn, topic, message, &block) end # Public: Print an error message @@ -78,8 +79,8 @@ def warn(topic, message = nil) # message - the message detail # # Returns nothing - def error(topic, message = nil) - writer.error(message(topic, message)) + def error(topic, message = nil, &block) + write(:error, topic, message, &block) end # Public: Print an error message and immediately abort the process @@ -88,8 +89,8 @@ def error(topic, message = nil) # message - the message detail (can be omitted) # # Returns nothing - def abort_with(topic, message = nil) - error(topic, message) + def abort_with(topic, message = nil, &block) + error(topic, message, &block) abort end @@ -99,19 +100,48 @@ def abort_with(topic, message = nil) # message - the message detail # # Returns the formatted message - def message(topic, message) - msg = formatted_topic(topic) + message.to_s.gsub(%r!\s+!, " ") - messages << msg - msg + def message(topic, message = nil) + raise ArgumentError, "block or message, not both" if block_given? && message + + message = yield if block_given? + message = message.to_s.gsub(%r!\s+!, " ") + topic = formatted_topic(topic, block_given?) + out = topic + message + messages << out + out end # Internal: Format the topic # # topic - the topic of the message, e.g. "Configuration file", "Deprecation", etc. + # colon - # # Returns the formatted topic statement - def formatted_topic(topic) - "#{topic} ".rjust(20) + def formatted_topic(topic, colon = false) + "#{topic}#{colon ? ": " : " "}".rjust(20) + end + + # Internal: Check if the message should be written given the log level. + # + # level_of_message - the Symbol level of message, one of :debug, :info, :warn, :error + # + # Returns whether the message should be written. + def write_message?(level_of_message) + LOG_LEVELS.fetch(level) <= LOG_LEVELS.fetch(level_of_message) + end + + # Internal: Log a message. + # + # level_of_message - the Symbol level of message, one of :debug, :info, :warn, :error + # topic - the String topic or full message + # message - the String message (optional) + # block - a block containing the message (optional) + # + # Returns false if the message was not written, otherwise returns the value of calling + # the appropriate writer method, e.g. writer.info. + def write(level_of_message, topic, message = nil, &block) + return false unless write_message?(level_of_message) + writer.public_send(level_of_message, message(topic, message, &block)) end end end diff --git a/test/helper.rb b/test/helper.rb index 3f3227bab4f..672fedd2862 100644 --- a/test/helper.rb +++ b/test/helper.rb @@ -32,7 +32,7 @@ def jruby? require "rspec/mocks" require_relative "../lib/jekyll.rb" -Jekyll.logger = Logger.new(StringIO.new) +Jekyll.logger = Logger.new(StringIO.new, :error) unless jruby? require "rdiscount" @@ -168,12 +168,15 @@ def with_env(key, value) ENV[key] = old_value end - def capture_output + def capture_output(level = :debug) buffer = StringIO.new Jekyll.logger = Logger.new(buffer) + Jekyll.logger.log_level = level yield buffer.rewind buffer.string.to_s + ensure + Jekyll.logger = Logger.new(StringIO.new, :error) end alias_method :capture_stdout, :capture_output alias_method :capture_stderr, :capture_output diff --git a/test/test_log_adapter.rb b/test/test_log_adapter.rb index 2bcd2f1d7c9..b15dca5020b 100644 --- a/test/test_log_adapter.rb +++ b/test/test_log_adapter.rb @@ -53,7 +53,7 @@ def error(*); end should "call #debug on writer return true" do writer = LoggerDouble.new - logger = Jekyll::LogAdapter.new(writer) + logger = Jekyll::LogAdapter.new(writer, :debug) allow(writer).to receive(:debug).and_return(true) assert logger.adjust_verbosity end @@ -62,7 +62,7 @@ def error(*); end context "#debug" do should "call #debug on writer return true" do writer = LoggerDouble.new - logger = Jekyll::LogAdapter.new(writer) + logger = Jekyll::LogAdapter.new(writer, :debug) allow(writer).to receive(:debug) .with("topic ".rjust(20) + "log message").and_return(true) assert logger.debug("topic", "log message") @@ -72,7 +72,7 @@ def error(*); end context "#info" do should "call #info on writer return true" do writer = LoggerDouble.new - logger = Jekyll::LogAdapter.new(writer) + logger = Jekyll::LogAdapter.new(writer, :info) allow(writer).to receive(:info) .with("topic ".rjust(20) + "log message").and_return(true) assert logger.info("topic", "log message") @@ -82,7 +82,7 @@ def error(*); end context "#warn" do should "call #warn on writer return true" do writer = LoggerDouble.new - logger = Jekyll::LogAdapter.new(writer) + logger = Jekyll::LogAdapter.new(writer, :warn) allow(writer).to receive(:warn) .with("topic ".rjust(20) + "log message").and_return(true) assert logger.warn("topic", "log message") @@ -92,7 +92,7 @@ def error(*); end context "#error" do should "call #error on writer return true" do writer = LoggerDouble.new - logger = Jekyll::LogAdapter.new(writer) + logger = Jekyll::LogAdapter.new(writer, :error) allow(writer).to receive(:error) .with("topic ".rjust(20) + "log message").and_return(true) assert logger.error("topic", "log message") @@ -101,7 +101,7 @@ def error(*); end context "#abort_with" do should "call #error and abort" do - logger = Jekyll::LogAdapter.new(LoggerDouble.new) + logger = Jekyll::LogAdapter.new(LoggerDouble.new, :error) allow(logger).to receive(:error).with("topic", "log message").and_return(true) assert_raises(SystemExit) { logger.abort_with("topic", "log message") } end @@ -113,7 +113,7 @@ def error(*); end end should "store each log value in the array" do - logger = Jekyll::LogAdapter.new(LoggerDouble.new) + logger = Jekyll::LogAdapter.new(LoggerDouble.new, :debug) values = %w(one two three four) logger.debug(values[0]) logger.info(values[1]) @@ -122,4 +122,14 @@ def error(*); end assert_equal values.map { |value| "#{value} ".rjust(20) }, logger.messages end end + + context "#write_message?" do + should "return false up to the desired logging level" do + subject = Jekyll::LogAdapter.new(LoggerDouble.new, :warn) + refute subject.write_message?(:debug), "Should not print debug messages" + refute subject.write_message?(:info), "Should not print info messages" + assert subject.write_message?(:warn), "Should print warn messages" + assert subject.write_message?(:error), "Should print error messages" + end + end end diff --git a/test/test_new_command.rb b/test/test_new_command.rb index c2b7def4001..632ad3d57c6 100644 --- a/test/test_new_command.rb +++ b/test/test_new_command.rb @@ -22,7 +22,7 @@ def site_template end teardown do - FileUtils.rm_r @full_path + FileUtils.rm_r @full_path if File.directory?(@full_path) end should "create a new directory" do @@ -41,8 +41,7 @@ def site_template end should "display a success message" do - Jekyll::Commands::New.process(@args) - output = Jekyll.logger.messages + output = capture_output { Jekyll::Commands::New.process(@args) } success_message = "New jekyll site installed in #{@full_path.cyan}. " bundle_message = "Running bundle install in #{@full_path.cyan}... " assert_includes output, success_message @@ -88,8 +87,7 @@ def site_template should "create blank project" do blank_contents = %w(/_drafts /_layouts /_posts /index.html) - capture_output { Jekyll::Commands::New.process(@args, "--blank") } - output = Jekyll.logger.messages.last + output = capture_output { Jekyll::Commands::New.process(@args, "--blank") } bundle_message = "Running bundle install in #{@full_path.cyan}..." assert_same_elements blank_contents, dir_contents(@full_path) refute_includes output, bundle_message @@ -98,12 +96,11 @@ def site_template should "force created folder" do capture_output { Jekyll::Commands::New.process(@args) } output = capture_output { Jekyll::Commands::New.process(@args, "--force") } - assert_match(%r!New jekyll site installed in!, output) + assert_match %r!New jekyll site installed in!, output end should "skip bundle install when opted to" do - capture_output { Jekyll::Commands::New.process(@args, "--skip-bundle") } - output = Jekyll.logger.messages.last + output = capture_output { Jekyll::Commands::New.process(@args, "--skip-bundle") } bundle_message = "Bundle install skipped." assert_includes output, bundle_message end diff --git a/test/test_site.rb b/test/test_site.rb index adc69e33436..f50990e0511 100644 --- a/test/test_site.rb +++ b/test/test_site.rb @@ -562,13 +562,13 @@ def convert(*_args) end expected_msg = "Theme: value of 'theme' in config should be String " \ "to use gem-based themes, but got Hash\n" - assert output.end_with?(expected_msg), - "Expected #{output.inspect} to end with #{expected_msg.inspect}" + assert_includes output, expected_msg end should "set a theme if the config is a string" do - expect($stderr).not_to receive(:puts) - expect($stdout).not_to receive(:puts) + [:debug, :info, :warn, :error].each do |level| + expect(Jekyll.logger.writer).not_to receive(level) + end site = fixture_site({ "theme" => "test-theme" }) assert_instance_of Jekyll::Theme, site.theme assert_equal "test-theme", site.theme.name