From 33b5580ef6b6c851beb6852e56659afea8bb0b04 Mon Sep 17 00:00:00 2001 From: Daniel Pittman Date: Tue, 19 Apr 2011 18:22:03 -0700 Subject: maint: move method comments outside the comment. The comment discussing the purpose of the wrapper and related details rightly belongs outside the method; move it there so it doesn't perturb the functional changes that follow. Reviewed-By: Max Martin --- lib/puppet/interface/action.rb | 39 +++++++++++++++++++-------------------- 1 file changed, 19 insertions(+), 20 deletions(-) (limited to 'lib/puppet/interface') diff --git a/lib/puppet/interface/action.rb b/lib/puppet/interface/action.rb index 23366b407..e644d8999 100644 --- a/lib/puppet/interface/action.rb +++ b/lib/puppet/interface/action.rb @@ -129,27 +129,26 @@ 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" -- cgit From 5d7ef5caf30a0c5b3253340c5f2722e51c56c75e Mon Sep 17 00:00:00 2001 From: Daniel Pittman Date: Tue, 19 Apr 2011 20:23:27 -0700 Subject: (#7062) better argument handling in the action wrapper methods MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- lib/puppet/interface/action.rb | 38 ++++++++++++++++++++++++++++++-------- 1 file changed, 30 insertions(+), 8 deletions(-) (limited to 'lib/puppet/interface') 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 = <