Merge da87b5e into d36ef06

lib/nanoc/base/feature.rb

@@ -92,3 +92,4 @@ def self.all_outdated
 end
 
 Nanoc::Feature.define('live_cmd', version: '4.8')
+Nanoc::Feature.define('sensible_stack_traces', version: '4.8')

lib/nanoc/cli/error_handler.rb

@@ -115,9 +115,15 @@ def write_compact_error(error, stream)
       stream.puts
       stream.puts 'Captain! We’ve been hit!'
 
-      write_error_message(stream, error)
-      write_item_rep(stream, error)
-      write_stack_trace(stream, error)
+      if forwards_stack_trace?
+        write_stack_trace(stream, error)
+        write_error_message(stream, error)
+        write_item_rep(stream, error)
+      else
+        write_error_message(stream, error)
+        write_item_rep(stream, error)
+        write_stack_trace(stream, error)
+      end
     end
 
     # Writes a verbose representation of the error on the given stream.
@@ -140,6 +146,14 @@ def write_verbose_error(error, stream)
       write_load_paths(stream, verbose: true)
     end
 
+    # @api private
+    def forwards_stack_trace?
+      feature_enabled = Nanoc::Feature.enabled?(Nanoc::Feature::SENSIBLE_STACK_TRACES)
+      ruby_2_5_used = ruby_version.start_with?('2.5')
+
+      feature_enabled || ruby_2_5_used
+    end
+
     protected
 
     # @return [Hash<String, Array>] A hash containing the gem names as keys and gem versions as value
@@ -236,6 +250,10 @@ def using_bundler?
       defined?(Bundler) && Bundler::SharedHelpers.in_bundle?
     end
 
+    def ruby_version
+      RUBY_VERSION
+    end
+
     def write_section_header(stream, title, verbose: false)
       stream.puts
 
@@ -271,7 +289,9 @@ def write_item_rep(stream, error, verbose: false)
 
     def write_stack_trace(stream, error, verbose: false)
       write_section_header(stream, 'Stack trace', verbose: verbose)
-      StackTraceWriter.new(stream).write(unwrap_error(error), verbose: verbose)
+
+      writer = StackTraceWriter.new(stream, forwards: forwards_stack_trace?)
+      writer.write(unwrap_error(error), verbose: verbose)
     end
 
     def write_issue_link(stream, _params = {})

lib/nanoc/cli/stack_trace_writer.rb

@@ -3,11 +3,22 @@
 module Nanoc::CLI
   # @api private
   class StackTraceWriter
-    def initialize(stream)
+    def initialize(stream, forwards:)
       @stream = stream
+      @forwards = forwards
     end
 
     def write(error, verbose:)
+      if @forwards
+        write_forwards(error, verbose: verbose)
+      else
+        write_backwards(error, verbose: verbose)
+      end
+    end
+
+    private
+
+    def write_backwards(error, verbose:)
       count = verbose ? -1 : 10
 
       error.backtrace[0...count].each_with_index do |item, index|
@@ -18,5 +29,22 @@ def write(error, verbose:)
         @stream.puts "  ... #{error.backtrace.size - count} lines omitted (see crash.log for details)"
       end
     end
+
+    def write_forwards(error, verbose:)
+      count = 10
+      backtrace = verbose ? error.backtrace : error.backtrace.take(count)
+
+      if !verbose && error.backtrace.size > count
+        @stream.puts "  ... #{error.backtrace.size - count} lines omitted (see crash.log for details)"
+      end
+
+      backtrace.each_with_index.to_a.reverse.each do |(item, index)|
+        if index.zero?
+          @stream.puts "  #{item}"
+        else
+          @stream.puts "  #{index}. from #{item}"
+        end
+      end
+    end
   end
 end

spec/nanoc/cli/error_handler_spec.rb

@@ -0,0 +1,43 @@
+# frozen_string_literal: true
+
+describe Nanoc::CLI::ErrorHandler do
+  subject(:error_handler) { described_class.new }
+
+  describe '#forwards_stack_trace?' do
+    subject { error_handler.forwards_stack_trace? }
+
+    context 'feature enabled' do
+      around do |ex|
+        Nanoc::Feature.enable(Nanoc::Feature::SENSIBLE_STACK_TRACES) do
+          ex.run
+        end
+      end
+
+      context 'Ruby 2.4' do
+        it { is_expected.to be(true) }
+      end
+
+      context 'Ruby 2.5' do
+        it { is_expected.to be(true) }
+      end
+    end
+
+    context 'feature not enabled' do
+      context 'Ruby 2.4' do
+        before do
+          expect(error_handler).to receive(:ruby_version).and_return('2.4.2')
+        end
+
+        it { is_expected.to be(false) }
+      end
+
+      context 'Ruby 2.5' do
+        before do
+          expect(error_handler).to receive(:ruby_version).and_return('2.5.0')
+        end
+
+        it { is_expected.to be(true) }
+      end
+    end
+  end
+end

spec/nanoc/cli/stack_trace_writer_spec.rb

@@ -1,82 +1,156 @@
 # frozen_string_literal: true
 
 describe Nanoc::CLI::StackTraceWriter do
-  let(:exception) do
-    backtrace_generator = lambda do |af|
-      if af.zero?
-        raise 'finally!'
-      else
-        backtrace_generator.call(af - 1)
+  subject(:writer) do
+    described_class.new(io, forwards: forwards)
+  end
+
+  let(:io) { StringIO.new }
+  let(:forwards) { true }
+
+  describe '#write' do
+    let(:exception) do
+      backtrace_generator = lambda do |af|
+        if af.zero?
+          raise 'finally!'
+        else
+          backtrace_generator.call(af - 1)
+        end
       end
-    end
 
-    begin
-      backtrace_generator.call(3)
-    rescue => e
-      return e
+      begin
+        backtrace_generator.call(3)
+      rescue => e
+        return e
+      end
     end
-  end
 
-  subject { described_class.new(io).write(exception, verbose: verbose) }
+    subject { writer.write(exception, verbose: verbose) }
 
-  let(:io) { StringIO.new }
-  let(:verbose) { false }
+    let(:verbose) { false }
 
-  context 'verbose' do
-    let(:verbose) { true }
+    context 'backwards' do
+      let(:forwards) { false }
 
-    it 'starts with zero' do
-      expect { subject }
-        .to change { io.string }
-        .from('')
-        .to(start_with('  0. '))
-    end
+      context 'verbose' do
+        let(:verbose) { true }
 
-    it 'has more recent stack frames at the top' do
-      expect { subject }
-        .to change { io.string }
-        .from('')
-        .to(match(%r{^  0\. /.+/spec/nanoc/cli/stack_trace_writer_spec\.rb:\d+.*$\n  1\. /.+/spec/nanoc/cli/stack_trace_writer_spec\.rb:\d}m))
-    end
+        it 'starts with zero' do
+          expect { subject }
+            .to change { io.string }
+            .from('')
+            .to(start_with('  0. '))
+        end
 
-    it 'has more than 10 stack frames' do
-      expect { subject }
-        .to change { io.string }
-        .from('')
-        .to(match(%r{^  11\. }))
-    end
+        it 'has more recent stack frames at the top' do
+          expect { subject }
+            .to change { io.string }
+            .from('')
+            .to(match(%r{^  0\. /.+/spec/nanoc/cli/stack_trace_writer_spec\.rb:\d+.*$\n  1\. /.+/spec/nanoc/cli/stack_trace_writer_spec\.rb:\d}m))
+        end
 
-    it 'does not contain a see-more explanation' do
-      subject
-      expect(io.string).not_to match(/crash\.log/)
-    end
-  end
+        it 'has more than 10 stack frames' do
+          expect { subject }
+            .to change { io.string }
+            .from('')
+            .to(match(%r{^  11\. }))
+        end
 
-  context 'not verbose' do
-    let(:verbose) { false }
+        it 'does not contain a see-more explanation' do
+          subject
+          expect(io.string).not_to match(/crash\.log/)
+        end
+      end
 
-    it 'starts with zero' do
-      expect { subject }
-        .to change { io.string }
-        .from('')
-        .to(start_with('  0. '))
-    end
+      context 'not verbose' do
+        let(:verbose) { false }
 
-    it 'has more recent stack frames at the top' do
-      expect { subject }
-        .to change { io.string }
-        .from('')
-        .to(match(%r{^  0\. /.+/spec/nanoc/cli/stack_trace_writer_spec\.rb:\d+.*$\n  1\. /.+/spec/nanoc/cli/stack_trace_writer_spec\.rb:\d}m))
-    end
+        it 'starts with zero' do
+          expect { subject }
+            .to change { io.string }
+            .from('')
+            .to(start_with('  0. '))
+        end
+
+        it 'has more recent stack frames at the top' do
+          expect { subject }
+            .to change { io.string }
+            .from('')
+            .to(match(%r{^  0\. /.+/spec/nanoc/cli/stack_trace_writer_spec\.rb:\d+.*$\n  1\. /.+/spec/nanoc/cli/stack_trace_writer_spec\.rb:\d}m))
+        end
 
-    it 'has not more than 10 stack frames' do
-      subject
-      expect(io.string).not_to match(/^  11\. /)
+        it 'has not more than 10 stack frames' do
+          subject
+          expect(io.string).not_to match(/^  11\. /)
+        end
+
+        it 'does not contain a see-more explanation' do
+          subject
+          expect(io.string).to include(" lines omitted (see crash.log for details)\n")
+        end
+      end
     end
 
-    it 'does not contain a see-more explanation' do
-      subject
-      expect(io.string).to include(" lines omitted (see crash.log for details)\n")
+    context 'forwards' do
+      let(:forwards) { true }
+
+      context 'verbose' do
+        let(:verbose) { true }
+
+        it 'ends with most recent line' do
+          expect { subject }
+            .to change { io.string }
+            .from('')
+            .to(match(%r{^  1\. from /.+/spec/nanoc/cli/stack_trace_writer_spec\.rb:\d+.*$\n  /.+/spec/nanoc/cli}m))
+        end
+
+        it 'has more recent stack frames at the bottom' do
+          expect { subject }
+            .to change { io.string }
+            .from('')
+            .to(match(%r{^  2\. from /.+/spec/nanoc/cli/stack_trace_writer_spec\.rb:\d+.*$\n  1\. from /.+/spec/nanoc/cli/stack_trace_writer_spec\.rb:\d}m))
+        end
+
+        it 'has more than 10 stack frames' do
+          expect { subject }
+            .to change { io.string }
+            .from('')
+            .to(match(%r{^  11\. from }))
+        end
+
+        it 'does not contain a see-more explanation' do
+          subject
+          expect(io.string).not_to match(/crash\.log/)
+        end
+      end
+
+      context 'not verbose' do
+        let(:verbose) { false }
+
+        it 'ends with most recent line' do
+          expect { subject }
+            .to change { io.string }
+            .from('')
+            .to(match(%r{^  1\. from /.+/spec/nanoc/cli/stack_trace_writer_spec\.rb:\d+.*$\n  /.+/spec/nanoc/cli}m))
+        end
+
+        it 'has more recent stack frames at the top' do
+          expect { subject }
+            .to change { io.string }
+            .from('')
+            .to(match(%r{^  2\. from /.+/spec/nanoc/cli/stack_trace_writer_spec\.rb:\d+.*$\n  1\. from /.+/spec/nanoc/cli/stack_trace_writer_spec\.rb:\d}m))
+        end
+
+        it 'has not more than 10 stack frames' do
+          subject
+          expect(io.string).not_to match(/^  11\. from /)
+        end
+
+        it 'does not contain a see-more explanation' do
+          subject
+          expect(io.string).to include(" lines omitted (see crash.log for details)\n")
+        end
+      end
     end
   end
 end