Allow yield to logger methods & bail early on no-op messages (#6315)

Merge pull request 6315

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

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

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

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

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