(#7062) better argument handling in the action wrapper methods

We previously used *args to collect all arguments to the action when_invoked
block, then tried vaguely to massage some little bits of them into the right
shape.

Methods defined with blocks, in Ruby 1.8, also have some fun behaviours.  The
most special is that if you pass more than one argument to a block defined
with only one Ruby will automatically coerce the arguments into an array – and
this is preserved when it is bound to a method.

This led to routine situations where we would pass the wrong number of
arguments to the block because, say, the user gave an extra argument on the
command line.

Instead of failing this would transmogrify the arguments in counterintuitive
ways, and end up with horrible stack traces when that interacted badly with
the code as written.

Now, instead, we work out the right argument format based on the arguments
that the when_invoked block takes.  This gives much better (albeit perhaps not
so user friendly) behaviour at the interface level.  Which is, at least,
consistent with other Ruby API.

Reviewed-By: Max Martin <max@puppetlabs.com>

1 files changed, 30 insertions, 8 deletions

diff --git a/lib/puppet/interface/action.rb b/lib/puppet/interface/action.rb
index e644d8999..08bc0a345 100644
--- a/lib/puppet/interface/action.rb
+++ b/lib/puppet/interface/action.rb
@@ -151,15 +151,37 @@ class Puppet::Interface::Action
   def when_invoked=(block)
 
     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)