From f3262e32700709a6998579a9f71cdc6b8f4a695e Mon Sep 17 00:00:00 2001 From: Conrad Irwin Date: Sat, 14 Jan 2012 00:33:36 -0800 Subject: [PATCH] Make exec_hook robust to exceptions The rationale for this is simple: you add hooks to enhance the core functionality with added niceness, so you don't want a buggy hook to break core functionality. If you call exec_hook via an instance of Pry, it will additionally print a warning to output whenever such an exception occurs, giving useful information on how to debug the problem. --- lib/pry/hooks.rb | 13 ++++++++++++- lib/pry/pry_class.rb | 2 +- lib/pry/pry_instance.rb | 27 +++++++++++++++++++++++---- test/test_hooks.rb | 36 ++++++++++++++++++++++++++++++++++++ 4 files changed, 72 insertions(+), 6 deletions(-) diff --git a/lib/pry/hooks.rb b/lib/pry/hooks.rb index cedcd665..09da224b 100644 --- a/lib/pry/hooks.rb +++ b/lib/pry/hooks.rb @@ -20,6 +20,10 @@ class Pry end protected :hooks + def errors + @errors ||= [] + end + # Destructively merge the contents of two `Pry:Hooks` instances. # @param [Pry::Hooks] other The `Pry::Hooks` instance to merge @@ -89,7 +93,14 @@ class Pry # silence warnings to get rid of 1.8's "warning: multiple values # for a block parameter" warnings Pry::Helpers::BaseHelpers.silence_warnings do - @hooks[event_name].map { |hook_name, callable| callable.call(*args, &block) }.last + @hooks[event_name].map do |hook_name, callable| + begin + callable.call(*args, &block) + rescue RescuableException => e + errors << e + e + end + end.last end end diff --git a/lib/pry/pry_class.rb b/lib/pry/pry_class.rb index 956342c6..6f79aeed 100644 --- a/lib/pry/pry_class.rb +++ b/lib/pry/pry_class.rb @@ -116,7 +116,7 @@ class Pry pry_instance.backtrace = caller.tap(&:shift) # yield the binding_stack to the hook for modification - Pry.config.hooks.exec_hook( + pry_instance.exec_hook( :when_started, binding_stack = [target], options, diff --git a/lib/pry/pry_instance.rb b/lib/pry/pry_instance.rb index 884bc643..76068b1b 100644 --- a/lib/pry/pry_instance.rb +++ b/lib/pry/pry_instance.rb @@ -142,7 +142,7 @@ class Pry # Initialize the repl session. # @param [Binding] target The target binding for the session. def repl_prologue(target) - hooks.exec_hook :before_session, output, target, self + exec_hook :before_session, output, target, self initialize_special_locals(target) @input_array << nil # add empty input so _in_ and _out_ match @@ -154,7 +154,7 @@ class Pry # Clean-up after the repl session. # @param [Binding] target The target binding for the session. def repl_epilogue(target) - hooks.exec_hook :after_session, output, target, self + exec_hook :after_session, output, target, self Pry.active_sessions -= 1 binding_stack.pop @@ -234,7 +234,7 @@ class Pry result = set_last_exception(e, target) ensure update_input_history(code) - hooks.exec_hook :after_eval, result, self + exec_hook :after_eval, result, self end # Perform a read. @@ -270,7 +270,7 @@ class Pry @suppress_output = true if eval_string =~ /;\Z/ || eval_string.empty? - hooks.exec_hook :after_read, eval_string, self + exec_hook :after_read, eval_string, self eval_string end @@ -402,6 +402,25 @@ class Pry Pry::Command::VOID_VALUE end + # Execute the specified hook. + # @param [Symbol] name The hook name to execute + # @param [*Object] args The arguments to pass to the hook + # @return [Object, Exception] The return value of the hook or the exception raised + # + # If executing a hook raises an exception, we log that and then continue sucessfully. + # To debug such errors, use the global variable $pry_hook_error, which is set as a + # result. + def exec_hook(name, *args, &block) + e_before = hooks.errors.size + hooks.exec_hook(name, *args, &block) + + hooks.errors[e_before..-1].each do |e| + output.puts "#{name} hook failed: #{e.class}: #{e.message}" + output.puts "#{e.backtrace.first}" + output.puts "(see _pry_.hooks.errors to debug)" + end + end + # Set the last result of an eval. # This method should not need to be invoked directly. # @param [Object] result The result. diff --git a/test/test_hooks.rb b/test/test_hooks.rb index a0acf7fe..836e7d1d 100644 --- a/test/test_hooks.rb +++ b/test/test_hooks.rb @@ -290,6 +290,21 @@ describe Pry::Hooks do @hooks.add_hook(:test_hook, :my_name3) { 3 } @hooks.exec_hook(:test_hook).should == 3 end + + it 'should add exceptions to the errors array' do + @hooks.add_hook(:test_hook, :foo1) { raise 'one' } + @hooks.add_hook(:test_hook, :foo2) { raise 'two' } + @hooks.add_hook(:test_hook, :foo3) { raise 'three' } + @hooks.exec_hook(:test_hook) + @hooks.errors.map(&:message).should == ['one', 'two', 'three'] + end + + it 'should return the last exception raised as the return value' do + @hooks.add_hook(:test_hook, :foo1) { raise 'one' } + @hooks.add_hook(:test_hook, :foo2) { raise 'two' } + @hooks.add_hook(:test_hook, :foo3) { raise 'three' } + @hooks.exec_hook(:test_hook).should == @hooks.errors.last + end end describe "integration tests" do @@ -335,6 +350,27 @@ describe Pry::Hooks do # cleanup after test Pry.config.exception_whitelist = old_ew end + + describe "exceptions" do + before do + Pry.config.hooks.add_hook(:after_eval, :baddums){ raise "Baddums" } + Pry.config.hooks.add_hook(:after_eval, :simbads){ raise "Simbads" } + end + + after do + Pry.config.hooks.delete_hook(:after_eval, :baddums) + Pry.config.hooks.delete_hook(:after_eval, :simbads) + end + it "should not raise exceptions" do + lambda{ + mock_pry("1", "2", "3") + }.should.not.raise + end + + it "should print out a notice for each exception raised" do + mock_pry("1").should =~ /after_eval hook failed: RuntimeError: Baddums\n.*after_eval hook failed: RuntimeError: Simbads/m + end + end end end