[Pkg-puppet-devel] [SCM] Puppet packaging for Debian branch, experimental, updated. debian/2.6.8-1-844-g7ec39d5

[Daniel Pittman]

The following commit has been merged in the experimental branch:
commit 3bb8bd30c039edaab0f9eb4ff6865ba5077bf549
Author: Daniel Pittman <daniel at puppetlabs.com>
Date:   Mon May 2 12:25:39 2011 -0700

    (#7317) better error handling in CLI face facade.
    
    The error handling code in the face CLI facade was pretty terrible: it make a
    bunch of assumptions about the structure of the error message it got back, and
    used exceptions to communicate inside the same function.
    
    Instead, we handle errors uniformly without raising and catching in the same
    method, report more nicely, and exit cleanly in all cases.
    
    Reviewed-By: Nick Lewis <nick at puppetlabs.com>

diff --git a/lib/puppet/application/face_base.rb b/lib/puppet/application/face_base.rb
index 69c3ad5..31e58ac 100644
--- a/lib/puppet/application/face_base.rb
+++ b/lib/puppet/application/face_base.rb
@@ -212,63 +212,56 @@ class Puppet::Application::FaceBase < Puppet::Application
     status = false
 
     # Call the method associated with the provided action (e.g., 'find').
-    if @action
-      begin
-        # We need to do arity checking here because this is generic code
-        # calling generic methods – that have argument defaulting.  We need to
-        # make sure we don't accidentally pass the options as the first
-        # argument to a method that takes one argument.  eg:
-        #
-        #   puppet facts find
-        #   => options => {}
-        #      @arguments => [{}]
-        #   => @face.send :bar, {}
-        #
-        #   def face.bar(argument, options = {})
-        #   => bar({}, {})  # oops!  we thought the options were the
-        #                   # positional argument!!
-        #
-        # We could also fix this by making it mandatory to pass the options on
-        # every call, but that would make the Ruby API much more annoying to
-        # work with; having the defaulting is a much nicer convention to have.
-        #
-        # We could also pass the arguments implicitly, by having a magic
-        # 'options' method that was visible in the scope of the action, which
-        # returned the right stuff.
-        #
-        # That sounds attractive, but adds complications to all sorts of
-        # things, especially when you think about how to pass options when you
-        # are writing Ruby code that calls multiple faces.  Especially if
-        # faces are involved in that. ;)
-        #
-        # --daniel 2011-04-27
-        if (arity = @action.positional_arg_count) > 0
-          unless (count = arguments.length) == arity then
-            raise ArgumentError, "wrong number of arguments (#{count} for #{arity})"
-          end
-        end
-
-        result = @face.send(@action.name, *arguments)
-        puts render(result) unless result.nil?
-        status = true
-      rescue Exception => detail
-        puts detail.backtrace if Puppet[:trace]
-
-        case detail
-        when ArgumentError then
-          got, want = /\((\d+) for (\d+)\)/.match(detail.to_s).to_a.map {|x| x.to_i }
-          Puppet.err "puppet #{@face.name} #{@action.name}: #{want} argument expected but #{got} given"
-          Puppet.err "Try 'puppet help #{@face.name} #{@action.name}' for usage"
+    unless @action
+      puts Puppet::Face[:help, :current].help(@face.name)
+      raise "#{face} does not respond to action #{arguments.first}"
+    end
 
-        else # generic exception handling, alas.
-          Puppet.err detail.to_s
-        end
+    # We need to do arity checking here because this is generic code
+    # calling generic methods – that have argument defaulting.  We need to
+    # make sure we don't accidentally pass the options as the first
+    # argument to a method that takes one argument.  eg:
+    #
+    #   puppet facts find
+    #   => options => {}
+    #      @arguments => [{}]
+    #   => @face.send :bar, {}
+    #
+    #   def face.bar(argument, options = {})
+    #   => bar({}, {})  # oops!  we thought the options were the
+    #                   # positional argument!!
+    #
+    # We could also fix this by making it mandatory to pass the options on
+    # every call, but that would make the Ruby API much more annoying to
+    # work with; having the defaulting is a much nicer convention to have.
+    #
+    # We could also pass the arguments implicitly, by having a magic
+    # 'options' method that was visible in the scope of the action, which
+    # returned the right stuff.
+    #
+    # That sounds attractive, but adds complications to all sorts of
+    # things, especially when you think about how to pass options when you
+    # are writing Ruby code that calls multiple faces.  Especially if
+    # faces are involved in that. ;)
+    #
+    # --daniel 2011-04-27
+    if (arity = @action.positional_arg_count) > 0
+      unless (count = arguments.length) == arity then
+        s = arity == 2 ? '' : 's'
+        raise ArgumentError, "puppet #{@face.name} #{@action.name} takes #{arity-1} argument#{s}, but you gave #{count-1}"
       end
-    else
-      puts "#{face} does not respond to action #{arguments.first}"
-      puts Puppet::Face[:help, :current].help(@face.name)
     end
 
+    result = @face.send(@action.name, *arguments)
+    puts render(result) unless result.nil?
+    status = true
+
+  rescue Exception => detail
+    puts detail.backtrace if Puppet[:trace]
+    Puppet.err detail.to_s
+    Puppet.err "Try 'puppet help #{@face.name} #{@action.name}' for usage"
+
+  ensure
     exit status
   end
 end
diff --git a/spec/unit/application/facts_spec.rb b/spec/unit/application/facts_spec.rb
index aed0426..2981dc8 100755
--- a/spec/unit/application/facts_spec.rb
+++ b/spec/unit/application/facts_spec.rb
@@ -11,8 +11,8 @@ describe Puppet::Application::Facts do
     subject.command_line.stubs(:args).returns %w{find}
     expect {
       expect { subject.run }.to exit_with 1
-    }.to have_printed /1 argument expected but 0 given/
-    @logs.first.to_s.should =~ /1 argument expected but 0 given/
+    }.to have_printed /err: puppet facts find takes 1 argument, but you gave 0/
+    @logs.first.to_s.should =~ /puppet facts find takes 1 argument, but you gave 0/
   end
 
   it "should return facts if a key is given to find" do

-- 
Puppet packaging for Debian