From c800c0f04ff869c23f45cbb2dcca59194b6b9b39 Mon Sep 17 00:00:00 2001
From: John Mair <jrmair@gmail.com>
Date: Wed, 23 Nov 2011 18:51:54 +1300
Subject: [PATCH] Hook system now executes hooks in order.

Also the return value of exec_hook() is now the value of the last executed hook.
Internally, hooks now use arrays (rather than hash). Also added a get_hooks() method.
Updated tests to reflect these changes.
---
 lib/pry/hooks.rb   | 102 +++++++++++++++++++++++++++++++--------------
 test/test_hooks.rb |  82 +++++++++++++++++++++++++++++-------
 2 files changed, 136 insertions(+), 48 deletions(-)

diff --git a/lib/pry/hooks.rb b/lib/pry/hooks.rb
index 1222ea38..ca23920f 100644
--- a/lib/pry/hooks.rb
+++ b/lib/pry/hooks.rb
@@ -6,17 +6,20 @@ class Pry
     end
 
     # Add a new hook to be executed for the `name` even.
-    # @param [Symbol] name The name of the event.
-    # @param [Symbol] hook_function_name The name of the hook.
+    # @param [Symbol] event_name The name of the event.
+    # @param [Symbol] hook_name The name of the hook.
     # @param [#call] callable The callable.
     # @yield The block to use as the callable (if `callable` parameter not provided)
-    def add_hook(name, hook_function_name, callable=nil, &block)
-      @hooks[name] ||= {}
+    def add_hook(event_name, hook_name, callable=nil, &block)
+      @hooks[event_name] ||= []
+
+      # do not allow duplicates
+      raise ArgumentError, "Hook with name '#{hook_name}' already defined!" if hook_exists?(event_name, hook_name)
 
       if block
-        @hooks[name][hook_function_name] = block
+        @hooks[event_name] << [hook_name, block]
       elsif callable
-        @hooks[name][hook_function_name] = callable
+        @hooks[event_name] << [hook_name, callable]
       else
         raise ArgumentError, "Must provide a block or callable."
       end
@@ -24,47 +27,82 @@ class Pry
       self
     end
 
-    # Execute the list of hooks for the `name` event.
-    # @param [Symbol] name The name of the event.
+    # Execute the list of hooks for the `event_name` event.
+    # @param [Symbol] event_name The name of the event.
     # @param [Array] args The arguments to pass to each hook function.
-    # @return [Hash] The return values for each of the hook functions.
-    def exec_hook(name, *args, &block)
-      @hooks[name] ||= {}
-      Hash[@hooks[name].each.map { |k, v| [k, v.call(*args, &block)] }]
+    # @return [Object] The return value of the last executed hook.
+    def exec_hook(event_name, *args, &block)
+      @hooks[event_name] ||= []
+
+      # 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
+      end
     end
 
-    # Return the number of hook functions registered for the `name` event.
-    # @param [Symbol] name The name of the event.
-    # @return [Fixnum] The number of hook functions for the `name` event.
-    def hook_count(name)
-      @hooks[name] ||= {}
-      @hooks[name].size
+    # Return the number of hook functions registered for the `event_name` event.
+    # @param [Symbol] event_name The name of the event.
+    # @return [Fixnum] The number of hook functions for `event_name`.
+    def hook_count(event_name)
+      @hooks[event_name] ||= []
+      @hooks[event_name].size
+    end
+
+    # Return a specific hook for a given event.
+    # @param [Symbol] event_name The name of the event.
+    # @param [Symbol[ hook_name The name of the hook
+    # @return [#call] The requested hook.
+    def get_hook(event_name, hook_name)
+      @hooks[event_name] ||= []
+      hook = @hooks[event_name].find { |current_hook_name, callable| current_hook_name == hook_name }
+      hook.last if hook
     end
 
     # Return the hash of hook names / hook functions for a
-    # given event.
-    # @param [Symbol] name The name of the event.
-    # @return [Hash]
-    def get_hook(name, hook_function_name)
-      @hooks[name] ||= {}
-      @hooks[name][hook_function_name]
+    # given event. (Note that modifying the returned hash does not
+    # alter the hooks, use add_hook/delete_hook for that).
+    # @param [Symbol] event_name The name of the event.
+    # @return [Hash] The hash of hook names / hook functions.
+    def get_hooks(event_name)
+      @hooks[event_name] ||= []
+      Hash[@hooks[event_name]]
     end
 
     # Delete a hook for an event.
-    # @param [Symbol] name The name of the event.
-    # @param [Symbol] hook_function_name The name of the hook.
+    # @param [Symbol] event_name The name of the event.
+    # @param [Symbol] hook_name The name of the hook.
     #   to delete.
     # @return [#call] The deleted hook.
-    def delete_hook(name, hook_function_name)
-      @hooks[name] ||= {}
-      @hooks[name].delete(hook_function_name)
+    def delete_hook(event_name, hook_name)
+      @hooks[event_name] ||= []
+      deleted_callable = nil
+
+      @hooks[event_name].delete_if do |current_hook_name, callable|
+        if current_hook_name == hook_name
+          deleted_callable = callable
+          true
+        else
+          false
+        end
+      end
+      deleted_callable
     end
 
     # Clear all hooks functions for a given event.
-    # @param [String] name The name of the event.
-    def clear(name)
-      @hooks[name] = {}
+    # @param [String] event_name The name of the event.
+    def clear(event_name)
+      @hooks[event_name] = []
     end
 
+    # @param [Symbol] event_name Name of the event.
+    # @param [Symbol] hook_name Name of the hook.
+    # @return [Boolean] Whether the hook by the name `hook_name`
+    #   is defined for the event.
+    def hook_exists?(event_name, hook_name)
+      !!@hooks[event_name].find { |name, _| name == hook_name }
+    end
+    private :hook_exists?
+
   end
 end
diff --git a/test/test_hooks.rb b/test/test_hooks.rb
index f60f7cac..2eddb06b 100644
--- a/test/test_hooks.rb
+++ b/test/test_hooks.rb
@@ -7,12 +7,15 @@ describe Pry::Hooks do
 
   describe "adding a new hook" do
     it 'should not execute hook while adding it' do
-      @hooks.add_hook(:test_hook, :my_name) { @test_var = true }
-      @test_var.should == nil
+      run = false
+      @hooks.add_hook(:test_hook, :my_name) { run = true }
+      run.should == false
     end
 
-    it 'should return a count of 0 for an empty hook' do
-      @hooks.hook_count(:test_hook).should == 0
+    it 'should not allow adding of a hook with a duplicate name' do
+      @hooks.add_hook(:test_hook, :my_name) {}
+
+      lambda { @hooks.add_hook(:test_hook, :my_name) {} }.should.raise ArgumentError
     end
 
     it 'should create a new hook with a block' do
@@ -20,12 +23,12 @@ describe Pry::Hooks do
       @hooks.hook_count(:test_hook).should == 1
     end
 
-    it 'should create a new hook for an event' do
+    it 'should create a new hook with a callable' do
       @hooks.add_hook(:test_hook, :my_name, proc { })
       @hooks.hook_count(:test_hook).should == 1
     end
 
-    it 'should use just block if given both block and callable' do
+    it 'should use block if given both block and callable' do
       run = false
       foo = false
       @hooks.add_hook(:test_hook, :my_name, proc { foo = true }) { run = true }
@@ -44,17 +47,44 @@ describe Pry::Hooks do
       @hooks.add_hook(:test_hook, :my_name2) {}
       @hooks.hook_count(:test_hook).should == 2
     end
+
+    it 'should return a count of 0 for an empty hook' do
+      @hooks.hook_count(:test_hook).should == 0
+    end
   end
 
-  describe "getting a hook" do
-    it 'should return the correct requested hook' do
-      run = false
-      fun = false
-      @hooks.add_hook(:test_hook, :my_name) { run = true }
-      @hooks.add_hook(:test_hook, :my_name2) { fun = true }
-      @hooks.get_hook(:test_hook, :my_name).call
-      run.should == true
-      fun.should == false
+  describe "getting hooks" do
+    describe "get_hook" do
+      it 'should return the correct requested hook' do
+        run = false
+        fun = false
+        @hooks.add_hook(:test_hook, :my_name) { run = true }
+        @hooks.add_hook(:test_hook, :my_name2) { fun = true }
+        @hooks.get_hook(:test_hook, :my_name).call
+        run.should == true
+        fun.should == false
+      end
+
+      it 'should return nil if hook does not exist' do
+        @hooks.get_hook(:test_hook, :my_name).should == nil
+      end
+    end
+
+    describe "get_hooks" do
+      it 'should return a hash of hook names/hook functions for an event' do
+        hook1 = proc { 1 }
+        hook2 = proc { 2 }
+        @hooks.add_hook(:test_hook, :my_name1, hook1)
+        @hooks.add_hook(:test_hook, :my_name2, hook2)
+        hash = @hooks.get_hooks(:test_hook)
+        hash.size.should == 2
+        hash[:my_name1].should == hook1
+        hash[:my_name2].should == hook2
+      end
+
+      it 'should return an empty hash if no hooks defined' do
+        @hooks.get_hooks(:test_hook).should == {}
+      end
     end
   end
 
@@ -81,6 +111,10 @@ describe Pry::Hooks do
       @hooks.delete_hook(:test_hook, :my_name).call
       run.should == true
     end
+
+    it 'should return nil if hook does not exist' do
+      @hooks.delete_hook(:test_hook, :my_name).should == nil
+    end
   end
 
   describe "executing a hook" do
@@ -115,11 +149,27 @@ describe Pry::Hooks do
     it 'should execute all hooks for an event if more than one is defined' do
       x = nil
       y = nil
+      @hooks.add_hook(:test_hook, :my_name1) { y = true }
       @hooks.add_hook(:test_hook, :my_name2) { x = true }
-      @hooks.add_hook(:test_hook, :my_name) { y = true }
       @hooks.exec_hook(:test_hook)
       x.should == true
       y.should == true
     end
+
+    it 'should execute hooks in order' do
+      array = []
+      @hooks.add_hook(:test_hook, :my_name1) { array << 1 }
+      @hooks.add_hook(:test_hook, :my_name2) { array << 2 }
+      @hooks.add_hook(:test_hook, :my_name3) { array << 3 }
+      @hooks.exec_hook(:test_hook)
+      array.should == [1, 2, 3]
+    end
+
+    it 'return value of exec_hook should be that of last executed hook' do
+      @hooks.add_hook(:test_hook, :my_name1) { 1 }
+      @hooks.add_hook(:test_hook, :my_name2) { 2 }
+      @hooks.add_hook(:test_hook, :my_name3) { 3 }
+      @hooks.exec_hook(:test_hook).should == 3
+    end
   end
 end