Merge branch 'bug/2.7.x/7062-improve-cloudpack-option-parsing-errors' into 2.7.x

2 files changed, 105 insertions, 28 deletions

diff --git a/lib/puppet/interface/action.rb b/lib/puppet/interface/action.rb
index 23366b407..08bc0a345 100644
--- a/lib/puppet/interface/action.rb
+++ b/lib/puppet/interface/action.rb
@@ -129,38 +129,59 @@ class Puppet::Interface::Action
   #   @face.send(name, *args, &block)
   # end
 
+
+  # We need to build an instance method as a wrapper, using normal code, to be
+  # able to expose argument defaulting between the caller and definer in the
+  # Ruby API.  An extra method is, sadly, required for Ruby 1.8 to work since
+  # it doesn't expose bind on a block.
+  #
+  # Hopefully we can improve this when we finally shuffle off the last of Ruby
+  # 1.8 support, but that looks to be a few "enterprise" release eras away, so
+  # we are pretty stuck with this for now.
+  #
+  # Patches to make this work more nicely with Ruby 1.9 using runtime version
+  # checking and all are welcome, provided that they don't change anything
+  # outside this little ol' bit of code and all.
+  #
+  # Incidentally, we though about vendoring evil-ruby and actually adjusting
+  # the internal C structure implementation details under the hood to make
+  # this stuff work, because it would have been cleaner.  Which gives you an
+  # idea how motivated we were to make this cleaner.  Sorry.
+  # --daniel 2011-03-31
   def when_invoked=(block)
-    # We need to build an instance method as a wrapper, using normal code, to
-    # be able to expose argument defaulting between the caller and definer in
-    # the Ruby API.  An extra method is, sadly, required for Ruby 1.8 to work.
-    #
-    # In future this also gives us a place to hook in additional behaviour
-    # such as calling out to the action instance to validate and coerce
-    # parameters, which avoids any exciting context switching and all.
-    #
-    # Hopefully we can improve this when we finally shuffle off the last of
-    # Ruby 1.8 support, but that looks to be a few "enterprise" release eras
-    # away, so we are pretty stuck with this for now.
-    #
-    # Patches to make this work more nicely with Ruby 1.9 using runtime
-    # version checking and all are welcome, but they can't actually help if
-    # the results are not totally hidden away in here.
-    #
-    # Incidentally, we though about vendoring evil-ruby and actually adjusting
-    # the internal C structure implementation details under the hood to make
-    # this stuff work, because it would have been cleaner.  Which gives you an
-    # idea how motivated we were to make this cleaner.  Sorry. --daniel 2011-03-31
 
     internal_name = "#{@name} implementation, required on Ruby 1.8".to_sym
-    file    = __FILE__ + "+eval"
-    line    = __LINE__ + 1
+
+    arity = block.arity
+    if arity == 0 then
+      # This will never fire on 1.8.7, which treats no arguments as "*args",
+      # but will on 1.9.2, which treats it as "no arguments".  Which bites,
+      # because this just begs for us to wind up in the horrible situation
+      # where a 1.8 vs 1.9 error bites our end users. --daniel 2011-04-19
+      raise ArgumentError, "action when_invoked requires at least one argument (options)"
+    elsif arity > 0 then
+      range = Range.new(1, arity - 1)
+      decl = range.map { |x| "arg#{x}" } << "options = {}"
+      optn = ""
+      args = "[" + (range.map { |x| "arg#{x}" } << "options").join(", ") + "]"
+    else
+      range = Range.new(1, arity.abs - 1)
+      decl = range.map { |x| "arg#{x}" } << "*rest"
+      optn = "rest << {} unless rest.last.is_a?(Hash)"
+      if arity == -1 then
+        args = "rest"
+      else
+        args = "[" + range.map { |x| "arg#{x}" }.join(", ") + "] + rest"
+      end
+    end
+
+    file    = __FILE__ + "+eval[wrapper]"
+    line    = __LINE__ + 2 # <== points to the same line as 'def' in the wrapper.
     wrapper = <<WRAPPER
-def #{@name}(*args)
-  if args.last.is_a? Hash then
-    options = args.last
-  else
-    args << (options = {})
-  end
+def #{@name}(#{decl.join(", ")})
+  #{optn}
+  args = #{args}
+  options = args.last
 
   action = get_action(#{name.inspect})
   action.validate_args(args)
diff --git a/spec/unit/interface/action_spec.rb b/spec/unit/interface/action_spec.rb
index 0eb450ee2..24826a6ef 100755
--- a/spec/unit/interface/action_spec.rb
+++ b/spec/unit/interface/action_spec.rb
@@ -12,6 +12,62 @@ describe Puppet::Interface::Action do
     end
   end
 
+  describe "#when_invoked=" do
+    it "should fail if the block has arity 0" do
+      pending "Ruby 1.8 (painfully) treats argument-free blocks as arity -1" if
+        RUBY_VERSION =~ /^1\.8/
+
+      expect {
+        Puppet::Interface.new(:action_when_invoked, '1.0.0') do
+          action :foo do
+            when_invoked do
+            end
+          end
+        end
+      }.to raise_error ArgumentError, /foobra/
+    end
+
+    it "should work with arity 1 blocks" do
+      face = Puppet::Interface.new(:action_when_invoked, '1.0.0') do
+        action :foo do
+          when_invoked {|one| }
+        end
+      end
+      # -1, because we use option defaulting. :(
+      face.method(:foo).arity.should == -1
+    end
+
+    it "should work with arity 2 blocks" do
+      face = Puppet::Interface.new(:action_when_invoked, '1.0.0') do
+        action :foo do
+          when_invoked {|one, two| }
+        end
+      end
+      # -2, because we use option defaulting. :(
+      face.method(:foo).arity.should == -2
+    end
+
+    it "should work with arity 1 blocks that collect arguments" do
+      face = Puppet::Interface.new(:action_when_invoked, '1.0.0') do
+        action :foo do
+          when_invoked {|*one| }
+        end
+      end
+      # -1, because we use only varargs
+      face.method(:foo).arity.should == -1
+    end
+
+    it "should work with arity 2 blocks that collect arguments" do
+      face = Puppet::Interface.new(:action_when_invoked, '1.0.0') do
+        action :foo do
+          when_invoked {|one, *two| }
+        end
+      end
+      # -2, because we take one mandatory argument, and one varargs
+      face.method(:foo).arity.should == -2
+    end
+  end
+
   describe "when invoking" do
     it "should be able to call other actions on the same object" do
       face = Puppet::Interface.new(:my_face, '0.0.1') do