From 20d9ac1a5b71cd52c3edea107c1cef08562641fd Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Sun, 10 Apr 2011 15:57:33 -0700
Subject: maint: fix indentation in the watchr script.

Whitespace changes, no functional changes.

Reviewed-By: Matt Robinson <matt@puppetlabs.com>
---
 spec/watchr.rb | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/spec/watchr.rb b/spec/watchr.rb
index bad89b088..6f952a64f 100644
--- a/spec/watchr.rb
+++ b/spec/watchr.rb
@@ -15,10 +15,10 @@ def run_comp(cmd)
         line << c
         if c == ?\n
           results << if RUBY_VERSION >= "1.9" then
-            line.join
-              else
-                line.pack "c*"
-                  end
+                       line.join
+                     else
+                       line.pack "c*"
+                     end
           line.clear
         end
       end
-- 
cgit 


From 6fcf03c52f98c6e4bcce85548d783a832eaa387b Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Sun, 10 Apr 2011 16:47:33 -0700
Subject: maint: added testing for Puppet::Faces#[]

We didn't do much testing here, which was vaguely reasonable when we didn't
distinguish it from the #define method.  Now they are split out we need to be
more careful about testing the right things.

Reviewed-By: Matt Robinson <matt@puppetlabs.com>
---
 spec/unit/interface_spec.rb | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/spec/unit/interface_spec.rb b/spec/unit/interface_spec.rb
index ea11b21ba..b25d06f51 100755
--- a/spec/unit/interface_spec.rb
+++ b/spec/unit/interface_spec.rb
@@ -1,3 +1,4 @@
+require 'spec_helper'
 require 'puppet/faces'
 require 'puppet/interface'
 
@@ -16,6 +17,20 @@ describe Puppet::Interface do
     Puppet::Interface::FaceCollection.instance_variable_set("@faces", @faces)
   end
 
+  describe "#[]" do
+    it "should fail when no version is requested" do
+      expect { subject[:huzzah] }.should raise_error ArgumentError
+    end
+
+    it "should raise an exception when the requested version is unavailable" do
+      expect { subject[:huzzah, '17.0.0'] }.should raise_error, Puppet::Error
+    end
+
+    it "should raise an exception when the requested face doesn't exist" do
+      expect { subject[:burrble_toot, :current] }.should raise_error, Puppet::Error
+    end
+  end
+
   describe "#define" do
     it "should register the face" do
       face = subject.define(:face_test_register, '0.0.1')
-- 
cgit 


From 8b13e2b586af60bdef73ac8119b19233c60903ac Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Sun, 10 Apr 2011 16:56:59 -0700
Subject: maint: watchr should respect personal account-wide defaults.

Previously the rspec.opts file would override any other options configured for
rspec and, by virtue of being on the command-line, would win against
configuration files.

Now, where a user configuration file exists in ~/.rspec we ignore the shipped
set of options.  This allows advanced users to customize without using the
default (and relatively nasty) default rpsec UI.

Reviewed-By: Matt Robinson <matt@puppetlabs.com>
---
 spec/watchr.rb | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/spec/watchr.rb b/spec/watchr.rb
index 6f952a64f..26919d1a1 100644
--- a/spec/watchr.rb
+++ b/spec/watchr.rb
@@ -80,7 +80,11 @@ end
 def run_spec_files(files)
   files = Array(files)
   return if files.empty?
-  opts = File.readlines('spec/spec.opts').collect { |l| l.chomp }.join(" ")
+  if File.exist?(File.expand_path("~/.rspec")) then
+    opts = ''                   # use the user defaults
+  else
+    opts = File.readlines('spec/spec.opts').collect { |l| l.chomp }.join(" ")
+  end
   run_spec("rspec #{opts} --tty #{files.join(' ')}")
 end
 
@@ -89,12 +93,12 @@ def run_all_tests
 end
 
 def run_all_specs
-  run_test("rake spec")
+  run_spec_files "spec"
 end
 
 def run_suite
-  run_all_tests
   run_all_specs
+  run_all_tests
 end
 
 watch('spec/spec_helper.rb') { run_all_specs }
-- 
cgit 


From a20810e6a822dca3a69a1abf903dfea55aa166c8 Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Sun, 10 Apr 2011 19:01:02 -0700
Subject: maint: direct people to the expected spec file...

Because the implementation is directly an alias to another file, the spec here
is empty.  Instead, direct people over to the spec file they should be looking
in.  Makes it easier for new developers at pretty much zero cast.

Reviewed-By: Matt Robinson <matt@puppetlabs.com>
---
 spec/unit/faces_spec.rb | 1 +
 1 file changed, 1 insertion(+)
 create mode 100644 spec/unit/faces_spec.rb

diff --git a/spec/unit/faces_spec.rb b/spec/unit/faces_spec.rb
new file mode 100644
index 000000000..b6c49d917
--- /dev/null
+++ b/spec/unit/faces_spec.rb
@@ -0,0 +1 @@
+# You should look at interface_spec.rb
-- 
cgit 


From 7228f580a22cc13dc66a93d81bf57a5bad80a73f Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Mon, 11 Apr 2011 11:42:25 -0700
Subject: maint: finish transition of application help to return strings.

Jesse made a change, in e1191f33defcaffec5900c7122a89ca75d3a9673, to
transition from printing and exiting in the help method up to returning the
help data to the caller.  This was part of eliminating rdoc usage from the
display of help to the user.

The cert application was missed, and still used the legacy "print and exit"
model; this cleans that up so it matches the rest of the code.

Paired-With: Matt Robinson <matt@puppetlabs.com>
---
 lib/puppet/application/cert.rb | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/lib/puppet/application/cert.rb b/lib/puppet/application/cert.rb
index cbd6fd610..c08775380 100644
--- a/lib/puppet/application/cert.rb
+++ b/lib/puppet/application/cert.rb
@@ -48,7 +48,7 @@ class Puppet::Application::Cert < Puppet::Application
   end
 
   def help
-    puts <<-HELP
+    <<-HELP
 
 puppet-cert(8) -- Manage certificates and requests
 ========
@@ -166,7 +166,6 @@ COPYRIGHT
 Copyright (c) 2011 Puppet Labs, LLC Licensed under the Apache 2.0 License
 
     HELP
-    exit
   end
 
   def main
-- 
cgit 


From dc2675df2805d1e2dbf3c50a49152bcfd78f922f Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Sun, 10 Apr 2011 17:50:36 -0700
Subject: (#6770) Improve test robustness against 'require'

We hit another situation where we crudely flushed the internal set of faces,
but require thought we had already loaded the file defining one.  This fooled
our autoloader and raised more annoying problems.

Reviewed-By: Matt Robinson <matt@puppetlabs.com>
---
 spec/unit/interface/face_collection_spec.rb | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/spec/unit/interface/face_collection_spec.rb b/spec/unit/interface/face_collection_spec.rb
index b83bd50d3..e6e03c3d2 100755
--- a/spec/unit/interface/face_collection_spec.rb
+++ b/spec/unit/interface/face_collection_spec.rb
@@ -8,9 +8,14 @@ describe Puppet::Interface::FaceCollection do
   # To avoid cross-pollution we have to save and restore both the hash
   # containing all the interface data, and the array used by require.  Restoring
   # both means that we don't leak side-effects across the code. --daniel 2011-04-06
+  #
+  # Worse luck, we *also* need to flush $" of anything defining a face,
+  # because otherwise we can cross-pollute from other test files and end up
+  # with no faces loaded, but the require value set true. --daniel 2011-04-10
   before :each do
     @original_faces    = subject.instance_variable_get("@faces").dup
     @original_required = $".dup
+    $".delete_if do |path| path =~ %r{/faces/.*\.rb$} end
     subject.instance_variable_get("@faces").clear
   end
 
@@ -75,18 +80,14 @@ describe Puppet::Interface::FaceCollection do
     end
 
     it "should attempt to load the default faces for the specified version :current" do
-      subject.expects(:require).never # except...
       subject.expects(:require).with('puppet/faces/fozzie')
       subject['fozzie', :current]
     end
   end
 
   describe "::face?" do
-    before :each do
-      subject.instance_variable_get("@faces")[:foo]['0.0.1'] = 10
-    end
-
     it "should return true if the faces specified is registered" do
+      subject.instance_variable_get("@faces")[:foo]['0.0.1'] = 10
       subject.face?("foo", '0.0.1').should == true
     end
 
-- 
cgit 


From d8dfb1f143ba3e602bec387508c8dad7aab00062 Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Sun, 10 Apr 2011 18:59:23 -0700
Subject: (#6962) Implement 'summary' for faces.

This adds the methods to the summary builder and runtime instance to support
setting and getting a summary of the face.  This is a short description used
to summarize the purpose of the face in help output.

For example, from the help face:

    "Displays help about puppet subcommands"

Reviewed-By: Matt Robinson <matt@puppetlabs.com>
---
 lib/puppet/interface.rb                        |  9 +++++++--
 spec/unit/application/indirection_base_spec.rb |  2 +-
 spec/unit/interface_spec.rb                    | 27 ++++++++++++++++++++++++--
 3 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/lib/puppet/interface.rb b/lib/puppet/interface.rb
index 07e27efa8..27b3584b9 100644
--- a/lib/puppet/interface.rb
+++ b/lib/puppet/interface.rb
@@ -68,8 +68,13 @@ class Puppet::Interface
     self.default_format = format.to_sym
   end
 
-  attr_accessor :type, :verb, :version, :arguments
-  attr_reader :name
+  attr_accessor :summary
+  def summary(value = nil)
+    @summary = value unless value.nil?
+    @summary
+  end
+
+  attr_reader :name, :version
 
   def initialize(name, version, &block)
     unless Puppet::Interface::FaceCollection.validate_version(version)
diff --git a/spec/unit/application/indirection_base_spec.rb b/spec/unit/application/indirection_base_spec.rb
index 10ebe8e3d..98eb3a118 100755
--- a/spec/unit/application/indirection_base_spec.rb
+++ b/spec/unit/application/indirection_base_spec.rb
@@ -13,7 +13,7 @@ face = Puppet::Faces::Indirector.define(:testindirection, '0.0.1') do
 end
 # REVISIT: This horror is required because we don't allow anything to be
 # :current except for if it lives on, and is loaded from, disk. --daniel 2011-03-29
-face.version = :current
+face.instance_variable_set('@version', :current)
 Puppet::Faces.register(face)
 ########################################################################
 
diff --git a/spec/unit/interface_spec.rb b/spec/unit/interface_spec.rb
index b25d06f51..7e6b7de77 100755
--- a/spec/unit/interface_spec.rb
+++ b/spec/unit/interface_spec.rb
@@ -43,13 +43,36 @@ describe Puppet::Interface do
     end
 
     it "should require a version number" do
-      expect { subject.define(:no_version) }.should raise_error ArgumentError
+      expect { subject.define(:no_version) }.to raise_error ArgumentError
+    end
+
+    it "should support summary builder and accessor methods" do
+      subject.new(:foo, '1.0.0').should respond_to(:summary).with(0).arguments
+      subject.new(:foo, '1.0.0').should respond_to(:summary=).with(1).arguments
+    end
+
+    it "should set the summary text" do
+      text = "hello, freddy, my little pal"
+      subject.define(:face_test_summary, '1.0.0') do
+        summary text
+      end
+      subject[:face_test_summary, '1.0.0'].summary.should == text
+    end
+
+    it "should support mutating the summary" do
+      text = "hello, freddy, my little pal"
+      subject.define(:face_test_summary, '1.0.0') do
+        summary text
+      end
+      subject[:face_test_summary, '1.0.0'].summary.should == text
+      subject[:face_test_summary, '1.0.0'].summary = text + text
+      subject[:face_test_summary, '1.0.0'].summary.should == text + text
     end
   end
 
   describe "#initialize" do
     it "should require a version number" do
-      expect { subject.new(:no_version) }.should raise_error ArgumentError
+      expect { subject.new(:no_version) }.to raise_error ArgumentError
     end
 
     it "should require a valid version number" do
-- 
cgit 


From 1b4d7a51d10b217c7f67f3876242fff6dc694faa Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Mon, 4 Apr 2011 16:46:09 -0700
Subject: (#6962) Create the basic shape of the help face.

This implements the basic help face, along with the start of the support
structures; we include the basic application, and the default help action that
just emits a listing of faces and other discovered stuff...

Reviewed-By: Matt Robinson <matt@puppetlabs.com>
---
 lib/puppet/application/help.rb |  7 ++++++
 lib/puppet/faces/help.rb       | 52 ++++++++++++++++++++++++++++++++++++++++++
 spec/unit/faces/help_spec.rb   | 44 +++++++++++++++++++++++++++++++++++
 3 files changed, 103 insertions(+)
 create mode 100644 lib/puppet/application/help.rb
 create mode 100644 lib/puppet/faces/help.rb
 create mode 100644 spec/unit/faces/help_spec.rb

diff --git a/lib/puppet/application/help.rb b/lib/puppet/application/help.rb
new file mode 100644
index 000000000..69905af27
--- /dev/null
+++ b/lib/puppet/application/help.rb
@@ -0,0 +1,7 @@
+require 'puppet/application/faces_base'
+
+class Puppet::Application::Help < Puppet::Application::FacesBase
+  def render(result)
+    puts result.join("\n")
+  end
+end
diff --git a/lib/puppet/faces/help.rb b/lib/puppet/faces/help.rb
new file mode 100644
index 000000000..e986d19b3
--- /dev/null
+++ b/lib/puppet/faces/help.rb
@@ -0,0 +1,52 @@
+require 'puppet/faces'
+
+Puppet::Faces.define(:help, '0.0.1') do
+  summary "Displays help about puppet subcommands"
+
+  action(:help) do
+    option "--version VERSION" do
+      desc "Which version of the interface to show help for"
+    end
+
+    when_invoked do |*args|
+      # Check our invocation, because we want varargs and can't do defaults
+      # yet.  REVISIT: when we do option defaults, and positional options, we
+      # should rewrite this to use those. --daniel 2011-04-04
+      options = args.pop
+      if options.nil? or args.length > 2 then
+        raise ArgumentError, "help only takes two (optional) arguments, a face name, and an action"
+      end
+
+      if options[:version] and options[:version] !~ /^current$/i then
+        version = options[:version]
+      else
+        version = :current
+      end
+
+      message = []
+      if args.length == 0 then
+        message << "Use: puppet [options] <subcommand> <action>"
+        message << ""
+        message << "Available commands, from Puppet Faces:"
+        Puppet::Faces.faces.sort.each do |name|
+          face = Puppet::Faces[name, :current]
+          message << format("  %-15s %s", face.name, 'REVISIT: face.desc')
+        end
+      else
+        face = Puppet::Faces[args[0].to_sym, version]
+        if args[1] then
+          action = face.get_action args[1].to_sym
+        else
+          action = nil
+        end
+
+        help = []
+        face.actions.each do |action|
+          help << "Action: #{action}"
+        end
+      end
+
+      message
+    end
+  end
+end
diff --git a/spec/unit/faces/help_spec.rb b/spec/unit/faces/help_spec.rb
new file mode 100644
index 000000000..5b611a0ad
--- /dev/null
+++ b/spec/unit/faces/help_spec.rb
@@ -0,0 +1,44 @@
+require 'spec_helper'
+require 'puppet/faces/help'
+
+describe Puppet::Faces[:help, '0.0.1'] do
+  it "should have a help action" do
+    subject.should be_action :help
+  end
+
+  it "should have a default action of help" do
+    pending "REVISIT: we don't support default actions yet"
+  end
+
+  it "should accept a call with no arguments" do
+    expect { subject.help() }.should_not raise_error
+  end
+
+  it "should accept a face name" do
+    expect { subject.help(:help) }.should_not raise_error
+  end
+
+  it "should accept a face and action name" do
+    expect { subject.help(:help, :help) }.should_not raise_error
+  end
+
+  it "should fail if more than a face and action are given" do
+    expect { subject.help(:help, :help, :for_the_love_of_god) }.
+      should raise_error ArgumentError
+  end
+
+  it "should treat :current and 'current' identically" do
+    subject.help(:help, :current).should ==
+      subject.help(:help, 'current')
+  end
+
+  it "should complain when the request version of a face is missing" do
+    expect { subject.help(:huzzah, :bar, :version => '17.0.0') }.
+      should raise_error Puppet::Error
+  end
+
+  it "should find a face by version" do
+    face = Puppet::Faces[:huzzah, :current]
+    subject.help(:huzzah, face.version).should == subject.help(:huzzah, :current)
+  end
+end
-- 
cgit 


From 4eccd53da90593fad1b929eeea3b5d7d252c553e Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Sun, 10 Apr 2011 19:58:43 -0700
Subject: (#6962) Implement Face#summary support for the help face.

We now use the summary information available in other faces as part of
emitting a list our list of available subcommands.  This is seldom used in
faces, but enough emit the information to prove the concept.

Reviewed-By: Matt Robinson <matt@puppetlabs.com>
---
 lib/puppet/faces/help.rb        |  4 ++--
 spec/lib/puppet/faces/huzzah.rb |  1 +
 spec/spec_helper.rb             |  7 +++++++
 spec/unit/faces/help_spec.rb    | 21 +++++++++++++++++++++
 4 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/lib/puppet/faces/help.rb b/lib/puppet/faces/help.rb
index e986d19b3..229a0dc81 100644
--- a/lib/puppet/faces/help.rb
+++ b/lib/puppet/faces/help.rb
@@ -27,10 +27,10 @@ Puppet::Faces.define(:help, '0.0.1') do
       if args.length == 0 then
         message << "Use: puppet [options] <subcommand> <action>"
         message << ""
-        message << "Available commands, from Puppet Faces:"
+        message << "Available subcommands, from Puppet Faces:"
         Puppet::Faces.faces.sort.each do |name|
           face = Puppet::Faces[name, :current]
-          message << format("  %-15s %s", face.name, 'REVISIT: face.desc')
+          message << format("  %-15s %s", face.name, face.summary)
         end
       else
         face = Puppet::Faces[args[0].to_sym, version]
diff --git a/spec/lib/puppet/faces/huzzah.rb b/spec/lib/puppet/faces/huzzah.rb
index 735004475..e86730250 100644
--- a/spec/lib/puppet/faces/huzzah.rb
+++ b/spec/lib/puppet/faces/huzzah.rb
@@ -1,4 +1,5 @@
 require 'puppet/faces'
 Puppet::Faces.define(:huzzah, '2.0.1') do
+  summary "life is a thing for celebration"
   action :bar do "is where beer comes from" end
 end
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index d28cb2504..1187c1caf 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -7,6 +7,7 @@ ARGV.clear
 require 'puppet'
 require 'mocha'
 gem 'rspec', '>=2.0.0'
+require 'rspec/expectations'
 
 # So everyone else doesn't have to include this base constant.
 module PuppetSpec
@@ -65,3 +66,9 @@ RSpec.configure do |config|
     GC.enable
   end
 end
+
+RSpec::Matchers.define :have_matching_element do |expected|
+  match do |actual|
+    actual.any? { |item| item =~ expected }
+  end
+end
diff --git a/spec/unit/faces/help_spec.rb b/spec/unit/faces/help_spec.rb
index 5b611a0ad..ad553dc3a 100644
--- a/spec/unit/faces/help_spec.rb
+++ b/spec/unit/faces/help_spec.rb
@@ -41,4 +41,25 @@ describe Puppet::Faces[:help, '0.0.1'] do
     face = Puppet::Faces[:huzzah, :current]
     subject.help(:huzzah, face.version).should == subject.help(:huzzah, :current)
   end
+
+  context "when listing subcommands" do
+    subject { Puppet::Faces[:help, :current].help }
+
+    # Check a precondition for the next block; if this fails you have
+    # something odd in your set of faces, and we skip testing things that
+    # matter. --daniel 2011-04-10
+    it "should have at least one face with a summary" do
+      Puppet::Faces.faces.should be_any do |name|
+        Puppet::Faces[name, :current].summary
+      end
+    end
+
+    Puppet::Faces.faces.each do |name|
+      face = Puppet::Faces[name, :current]
+      summary = face.summary
+
+      it { should have_matching_element %r{ #{name} } }
+      it { should have_matching_element %r{ #{name} +#{summary}} } if summary
+    end
+  end
 end
-- 
cgit 


From 14b1e008c1368e6c56d68f83999472351fd3327a Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Mon, 11 Apr 2011 10:57:06 -0700
Subject: (#6992) Expose available_subcommands as a class method.

We previously treated the list of legacy commands as an instance property of
Puppet::Util::CommandLine.  This made it difficult to obtain that information
outside the context of the instantiated application.

In the longer term this should wither and die as we eliminate the legacy
applications entirely, but until then exposing that on the class is the
lightest mechanism for making that useful.

Paired-With: Matt Robinson <matt@puppetlabs.com>
---
 lib/puppet/util/command_line.rb     | 11 +++++++++--
 spec/unit/util/command_line_spec.rb |  5 +++++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/lib/puppet/util/command_line.rb b/lib/puppet/util/command_line.rb
index 52b5f81ef..2891030df 100644
--- a/lib/puppet/util/command_line.rb
+++ b/lib/puppet/util/command_line.rb
@@ -33,14 +33,21 @@ module Puppet
         File.join('puppet', 'application')
       end
 
-      def available_subcommands
-        absolute_appdirs = $LOAD_PATH.collect do |x| 
+      def self.available_subcommands
+        absolute_appdirs = $LOAD_PATH.collect do |x|
           File.join(x,'puppet','application')
         end.select{ |x| File.directory?(x) }
         absolute_appdirs.inject([]) do |commands, dir|
           commands + Dir[File.join(dir, '*.rb')].map{|fn| File.basename(fn, '.rb')}
         end.uniq
       end
+      # available_subcommands was previously an instance method, not a class
+      # method, and we have an unknown number of user-implemented applications
+      # that depend on that behaviour.  Forwarding allows us to preserve a
+      # backward compatible API. --daniel 2011-04-11
+      def available_subcommands
+        self.class.available_subcommands
+      end
 
       def usage_message
         usage = "Usage: puppet command <space separated arguments>"
diff --git a/spec/unit/util/command_line_spec.rb b/spec/unit/util/command_line_spec.rb
index ca5f0540f..c5c6e5f3e 100755
--- a/spec/unit/util/command_line_spec.rb
+++ b/spec/unit/util/command_line_spec.rb
@@ -111,6 +111,11 @@ describe Puppet::Util::CommandLine do
       @core_apps = %w{describe filebucket kick queue resource agent cert apply doc master}
       @command_line = Puppet::Util::CommandLine.new("foo", %w{ client --help whatever.pp }, @tty )
     end
+    it "should expose available_subcommands as a class method" do
+      @core_apps.each do |command|
+        @command_line.available_subcommands.should include command
+      end
+    end
     it 'should be able to find all existing commands' do
       @core_apps.each do |command|
         @command_line.available_subcommands.should include command
-- 
cgit 


From 36021021b4cb11aea0a5acd35d051db52d8fc99f Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Mon, 11 Apr 2011 11:19:33 -0700
Subject: (#6770) Don't pollute valid face list when #face? is called.

We had two conflicting uses of the list of available faces: in the #face?
method we were perfectly happy to create a top level key on any request, but
didn't populate the version set.

Meanwhile, in #faces we treated the set of top level keys as the absolute and
correct list of all *valid* faces, leading to pain and suffering when people
queried for an invalid face, but then expected to enumerate only valid faces.

Paired-With: Matt Robinson <matt@puppetlabs.com>
---
 lib/puppet/interface/face_collection.rb     | 12 ++++++++++--
 spec/unit/interface/face_collection_spec.rb |  5 +++++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/lib/puppet/interface/face_collection.rb b/lib/puppet/interface/face_collection.rb
index 84296582c..e4eb22fa3 100644
--- a/lib/puppet/interface/face_collection.rb
+++ b/lib/puppet/interface/face_collection.rb
@@ -53,7 +53,11 @@ module Puppet::Interface::FaceCollection
 
   def self.face?(name, version)
     name = underscorize(name)
-    return true if @faces[name].has_key?(version)
+
+    # Note: be careful not to accidentally create the top level key, either,
+    # because it will result in confusion when people try to enumerate the
+    # list of valid faces later. --daniel 2011-04-11
+    return true if @faces.has_key?(name) and @faces[name].has_key?(version)
 
     # We always load the current version file; the common case is that we have
     # the expected version and any compatibility versions in the same file,
@@ -106,7 +110,11 @@ module Puppet::Interface::FaceCollection
     # but we don't need that right now.
     #
     # So, this comment is a place-holder for that.  --daniel 2011-04-06
-    return !! @faces[name].has_key?(version)
+    #
+    # Note: be careful not to accidentally create the top level key, either,
+    # because it will result in confusion when people try to enumerate the
+    # list of valid faces later. --daniel 2011-04-11
+    return !! (@faces.has_key?(name) and @faces[name].has_key?(version))
   end
 
   def self.register(face)
diff --git a/spec/unit/interface/face_collection_spec.rb b/spec/unit/interface/face_collection_spec.rb
index e6e03c3d2..752871035 100755
--- a/spec/unit/interface/face_collection_spec.rb
+++ b/spec/unit/interface/face_collection_spec.rb
@@ -138,6 +138,11 @@ describe Puppet::Interface::FaceCollection do
         subject.face?(:huzzah, :current).should be_true
       end
     end
+
+    it "should not cause an invalid face to be enumerated later" do
+      subject.face?(:there_is_no_face, :current).should be_false
+      subject.faces.should_not include :there_is_no_face
+    end
   end
 
   describe "::register" do
-- 
cgit 


From 26db6456d22b95486646ae5b8b001acb2051c4da Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Mon, 11 Apr 2011 11:26:25 -0700
Subject: (#6962) render prints the rval; fix help subcommand.

The face application base uses render to transform the returned object to a
form where #to_s produces the output intended for the end user; we were
actually printing in the method instead, leading to an extraneous 'nil' at the
end of the output...

Paired-With: Matt Robinson <matt@puppetlabs.com>
---
 lib/puppet/application/help.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/puppet/application/help.rb b/lib/puppet/application/help.rb
index 69905af27..b3666f12d 100644
--- a/lib/puppet/application/help.rb
+++ b/lib/puppet/application/help.rb
@@ -2,6 +2,6 @@ require 'puppet/application/faces_base'
 
 class Puppet::Application::Help < Puppet::Application::FacesBase
   def render(result)
-    puts result.join("\n")
+    result.join("\n")
   end
 end
-- 
cgit 


From d13a938b0abc604a7802cf41ff75a30ad6ccd192 Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Mon, 11 Apr 2011 10:46:08 -0700
Subject: (#6962) Initial support for legacy applications in help.

We now enumerate and print the list of legacy applications in the unadorned
help action; this allows us a path to migrating forward smoothly while still
providing a good user experience.

Paired-With: Matt Robinson <matt@puppetlabs.com>
---
 lib/puppet/faces/help.rb     | 16 +++++++++++++++-
 spec/unit/faces/help_spec.rb |  4 ++++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/lib/puppet/faces/help.rb b/lib/puppet/faces/help.rb
index 229a0dc81..c284433d1 100644
--- a/lib/puppet/faces/help.rb
+++ b/lib/puppet/faces/help.rb
@@ -1,6 +1,9 @@
 require 'puppet/faces'
+require 'puppet/util/command_line'
 
 Puppet::Faces.define(:help, '0.0.1') do
+  HelpSummaryFormat = '  %-18s  %s'
+
   summary "Displays help about puppet subcommands"
 
   action(:help) do
@@ -30,7 +33,18 @@ Puppet::Faces.define(:help, '0.0.1') do
         message << "Available subcommands, from Puppet Faces:"
         Puppet::Faces.faces.sort.each do |name|
           face = Puppet::Faces[name, :current]
-          message << format("  %-15s %s", face.name, face.summary)
+          message << format(HelpSummaryFormat, face.name, face.summary)
+        end
+
+        legacy = Puppet::Util::CommandLine.available_subcommands.reject do |appname|
+          Puppet::Faces.face? appname.to_sym, :current
+        end
+        unless legacy.empty? then # great victory when this is true!
+          message << ""
+          message << "Available applications, soon to be ported to Faces:"
+          legacy.sort.each do |appname|
+            message << format(HelpSummaryFormat, appname, 'REVISIT: how to summarize these?')
+          end
         end
       else
         face = Puppet::Faces[args[0].to_sym, version]
diff --git a/spec/unit/faces/help_spec.rb b/spec/unit/faces/help_spec.rb
index ad553dc3a..87ff67948 100644
--- a/spec/unit/faces/help_spec.rb
+++ b/spec/unit/faces/help_spec.rb
@@ -61,5 +61,9 @@ describe Puppet::Faces[:help, '0.0.1'] do
       it { should have_matching_element %r{ #{name} } }
       it { should have_matching_element %r{ #{name} +#{summary}} } if summary
     end
+
+    Puppet::Util::CommandLine.available_subcommands do |name|
+      it { should have_matching_element %r{ #{name} } }
+    end
   end
 end
-- 
cgit 


From 91c29a72e2b728e2d9ba495cd34b7354faa3852b Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Mon, 11 Apr 2011 12:00:06 -0700
Subject: (#6962) Extract summary from legacy applications for help.

We use a dubious, but effective, regexp match on the existing man(1) style
help string for the application to extract the summary data.  This should
properly be implemented as a summary method down in the applications
themselves...

Paired-With: Matt Robinson <matt@puppetlabs.com>
---
 lib/puppet/faces/help.rb     | 39 +++++++++++++++++++++++++++++++++------
 spec/unit/faces/help_spec.rb | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 67 insertions(+), 6 deletions(-)

diff --git a/lib/puppet/faces/help.rb b/lib/puppet/faces/help.rb
index c284433d1..2eb2869a4 100644
--- a/lib/puppet/faces/help.rb
+++ b/lib/puppet/faces/help.rb
@@ -36,14 +36,12 @@ Puppet::Faces.define(:help, '0.0.1') do
           message << format(HelpSummaryFormat, face.name, face.summary)
         end
 
-        legacy = Puppet::Util::CommandLine.available_subcommands.reject do |appname|
-          Puppet::Faces.face? appname.to_sym, :current
-        end
-        unless legacy.empty? then # great victory when this is true!
+        unless legacy_applications.empty? then # great victory when this is true!
           message << ""
           message << "Available applications, soon to be ported to Faces:"
-          legacy.sort.each do |appname|
-            message << format(HelpSummaryFormat, appname, 'REVISIT: how to summarize these?')
+          legacy_applications.each do |appname|
+            summary = horribly_extract_summary_from appname
+            message << format(HelpSummaryFormat, appname, summary)
           end
         end
       else
@@ -63,4 +61,33 @@ Puppet::Faces.define(:help, '0.0.1') do
       message
     end
   end
+
+  def legacy_applications
+    # The list of applications, less those that are duplicated as a face.
+    Puppet::Util::CommandLine.available_subcommands.reject do |appname|
+      Puppet::Faces.face? appname.to_sym, :current or
+        # ...this is a nasty way to exclude non-applications. :(
+        %w{faces_base indirection_base}.include? appname
+    end.sort
+  end
+
+  def horribly_extract_summary_from(appname)
+    begin
+      require "puppet/application/#{appname}"
+      help = Puppet::Application[appname].help.split("\n")
+      # Now we find the line with our summary, extract it, and return it.  This
+      # depends on the implementation coincidence of how our pages are
+      # formatted.  If we can't match the pattern we expect we return the empty
+      # string to ensure we don't blow up in the summary. --daniel 2011-04-11
+      while line = help.shift do
+        if md = /^puppet-#{appname}\([^\)]+\) -- (.*)$/.match(line) then
+          return md[1]
+        end
+      end
+    rescue Exception
+      # Damn, but I hate this: we just ignore errors here, no matter what
+      # class they are.  Meh.
+    end
+    return ''
+  end
 end
diff --git a/spec/unit/faces/help_spec.rb b/spec/unit/faces/help_spec.rb
index 87ff67948..1399abfef 100644
--- a/spec/unit/faces/help_spec.rb
+++ b/spec/unit/faces/help_spec.rb
@@ -66,4 +66,38 @@ describe Puppet::Faces[:help, '0.0.1'] do
       it { should have_matching_element %r{ #{name} } }
     end
   end
+
+  context "when listing legacy applications" do
+    let :help do Puppet::Faces[:help, :current] end
+
+    # If we don't, these tests are ... less than useful, because they assume
+    # it.  When this breaks you should consider ditching the entire feature
+    # and tests, but if not work out how to fake one. --daniel 2011-04-11
+    it "should have at least one legacy application" do
+      help.legacy_applications.should have_at_least(1).item
+    end
+
+    # Meh.  This is nasty, but we can't control the other list; the specific
+    # bug that caused these to be listed is annoyingly subtle and has a nasty
+    # fix, so better to have a "fail if you do something daft" trigger in
+    # place here, I think. --daniel 2011-04-11
+    %w{faces_base indirection_base}.each do |name|
+      it "should not list the #{name} application" do
+        help.legacy_applications.should_not include name
+      end
+    end
+
+    Puppet::Faces[:help, :current].legacy_applications.each do |appname|
+      it "should list #{appname} in the help output" do
+        help.help.should have_matching_element %r{ #{appname} }
+      end
+
+      summary = Puppet::Faces[:help, :current].horribly_extract_summary_from(appname)
+      if summary then
+        it "should display the summary of #{appname}" do
+          help.help.should have_matching_element %r{ #{summary}\b}
+        end
+      end
+    end
+  end
 end
-- 
cgit 


From cdc5fec3640108ad01e0285b1039dae222590339 Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Mon, 11 Apr 2011 15:50:24 -0700
Subject: (#6962) Implement 'summary' for actions.

This extends the summary function down through the actions themselves,
allowing us to display a useful summary to the user.

Reviewed-By: Matt Robinson <matt@puppetlabs.com>
---
 lib/puppet/interface/action.rb             |  1 +
 lib/puppet/interface/action_builder.rb     |  4 ++++
 spec/unit/interface/action_builder_spec.rb | 11 +++++++++++
 3 files changed, 16 insertions(+)

diff --git a/lib/puppet/interface/action.rb b/lib/puppet/interface/action.rb
index e4a37a1f7..302e61901 100644
--- a/lib/puppet/interface/action.rb
+++ b/lib/puppet/interface/action.rb
@@ -14,6 +14,7 @@ class Puppet::Interface::Action
   attr_reader :name
   def to_s() "#{@face}##{@name}" end
 
+  attr_accessor :summary
 
   # Initially, this was defined to allow the @action.invoke pattern, which is
   # a very natural way to invoke behaviour given our introspection
diff --git a/lib/puppet/interface/action_builder.rb b/lib/puppet/interface/action_builder.rb
index b08c3d023..34bb3fa44 100644
--- a/lib/puppet/interface/action_builder.rb
+++ b/lib/puppet/interface/action_builder.rb
@@ -28,4 +28,8 @@ class Puppet::Interface::ActionBuilder
     option = Puppet::Interface::OptionBuilder.build(@action, *declaration, &block)
     @action.add_option(option)
   end
+
+  def summary(text)
+    @action.summary = text
+  end
 end
diff --git a/spec/unit/interface/action_builder_spec.rb b/spec/unit/interface/action_builder_spec.rb
index 7d2710942..666575605 100755
--- a/spec/unit/interface/action_builder_spec.rb
+++ b/spec/unit/interface/action_builder_spec.rb
@@ -55,5 +55,16 @@ describe Puppet::Interface::ActionBuilder do
         action.should be_option :bar
       end
     end
+
+    context "inline documentation" do
+      let :face do Puppet::Interface.new(:inline_action_docs, '0.0.1') end
+
+      it "should set the summary" do
+        action = Puppet::Interface::ActionBuilder.build(face, :foo) do
+          summary "this is some text"
+        end
+        action.summary.should == "this is some text"
+      end
+    end
   end
 end
-- 
cgit 


From 657082755a20da801b4c679eff296053380c61b6 Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Mon, 11 Apr 2011 16:19:21 -0700
Subject: (#6962) Add summary help for actions on an individual face.

We now emit the summary of actions for an individual face, in the same format
as the summary of available faces.  This moves forward through the feature set
defined for the help subcommand.

Reviewed-By: Matt Robinson <matt@puppetlabs.com>
---
 lib/puppet/faces/help.rb     | 54 ++++++++++++++++++++++++++++++++------------
 spec/unit/faces/help_spec.rb |  2 ++
 2 files changed, 41 insertions(+), 15 deletions(-)

diff --git a/lib/puppet/faces/help.rb b/lib/puppet/faces/help.rb
index 2eb2869a4..17ce3ec6b 100644
--- a/lib/puppet/faces/help.rb
+++ b/lib/puppet/faces/help.rb
@@ -7,6 +7,8 @@ Puppet::Faces.define(:help, '0.0.1') do
   summary "Displays help about puppet subcommands"
 
   action(:help) do
+    summary "Display help about faces and their actions."
+
     option "--version VERSION" do
       desc "Which version of the interface to show help for"
     end
@@ -20,15 +22,27 @@ Puppet::Faces.define(:help, '0.0.1') do
         raise ArgumentError, "help only takes two (optional) arguments, a face name, and an action"
       end
 
-      if options[:version] and options[:version] !~ /^current$/i then
-        version = options[:version]
-      else
-        version = :current
+      version = :current
+      if options.has_key? :version then
+        if options[:version].to_s !~ /^current$/i then
+          version = options[:version]
+        else
+          if args.length == 0 then
+            raise ArgumentError, "version only makes sense when a face is given"
+          end
+        end
       end
 
+      # Name those parameters...
+      facename, actionname = args
+      face   = facename ? Puppet::Faces[facename.to_sym, version] : nil
+      action = (face and actionname) ? face.get_action(actionname.to_sym) : nil
+
+      # Finally, build up the help text.  Maybe ERB would have been nicer
+      # after all.  Oh, well. --daniel 2011-04-11
       message = []
       if args.length == 0 then
-        message << "Use: puppet [options] <subcommand> <action>"
+        message << "Use: puppet <subcommand> [options] <action> [options]"
         message << ""
         message << "Available subcommands, from Puppet Faces:"
         Puppet::Faces.faces.sort.each do |name|
@@ -44,18 +58,28 @@ Puppet::Faces.define(:help, '0.0.1') do
             message << format(HelpSummaryFormat, appname, summary)
           end
         end
-      else
-        face = Puppet::Faces[args[0].to_sym, version]
-        if args[1] then
-          action = face.get_action args[1].to_sym
-        else
-          action = nil
-        end
 
-        help = []
-        face.actions.each do |action|
-          help << "Action: #{action}"
+        message << ""
+        message << <<EOT.split("\n")
+See 'puppet help <subcommand> <action>' for help on a specific subcommand action.
+See 'puppet help <subcommand>' for help on a specific subcommand.
+See 'puppet man  <subcommand>' for the full man page.
+Puppet v#{Puppet::PUPPETVERSION}
+EOT
+      elsif args.length == 1 then
+        message << "Use: puppet #{face.name} [options] <action> [options]"
+        message << ""
+
+        message << "Available actions:"
+        face.actions.each do |actionname|
+          action = face.get_action(actionname)
+          message << format(HelpSummaryFormat, action.name, action.summary)
         end
+
+      elsif args.length == 2
+        "REVISIT: gotta write this code."
+      else
+        raise ArgumentError, "help only takes two arguments, a face name and an action"
       end
 
       message
diff --git a/spec/unit/faces/help_spec.rb b/spec/unit/faces/help_spec.rb
index 1399abfef..61f1947f3 100644
--- a/spec/unit/faces/help_spec.rb
+++ b/spec/unit/faces/help_spec.rb
@@ -100,4 +100,6 @@ describe Puppet::Faces[:help, '0.0.1'] do
       end
     end
   end
+
+  
 end
-- 
cgit 


From 2a87f410eae84a0a7c4f39d9c1ef742c3e7ba8fc Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Mon, 11 Apr 2011 16:43:56 -0700
Subject: (#6962) Override 'render' in help to just return the string.

The default behaviour is to serialize the result somehow, defaulting to
displayed, render should be a no-op.  This means overriding the parent method.

Paired-With: Matt Robinson <matt@puppetlabs.com>
---
 lib/puppet/application/help.rb | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/lib/puppet/application/help.rb b/lib/puppet/application/help.rb
index b3666f12d..fd8818db0 100644
--- a/lib/puppet/application/help.rb
+++ b/lib/puppet/application/help.rb
@@ -1,7 +1,8 @@
+# -*- coding: utf-8 -*-
 require 'puppet/application/faces_base'
 
 class Puppet::Application::Help < Puppet::Application::FacesBase
-  def render(result)
-    result.join("\n")
-  end
+  # Meh.  Disable the default behaviour, which is to inspect the
+  # string and return that – not so helpful. --daniel 2011-04-11
+  def render(result) result end
 end
-- 
cgit 


From ec988e24ce3713a4c4a31918489136f88b6945e6 Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Mon, 11 Apr 2011 16:47:16 -0700
Subject: (#6962) Move the logic for help layout into erb templates.

Rather than hard-coding the layout of help output in the code, push it out
into external templates.  At the moment overriding that would require
changing the ERB code next to puppet/faces/help.rb file on disk.

Paired-With: Matt Robinson <matt@puppetlabs.com>
---
 lib/puppet/faces/help.rb         | 60 ++++++++++------------------------------
 lib/puppet/faces/help/action.erb |  3 ++
 lib/puppet/faces/help/face.erb   |  7 +++++
 lib/puppet/faces/help/global.erb | 20 ++++++++++++++
 spec/unit/faces/help_spec.rb     |  2 --
 5 files changed, 44 insertions(+), 48 deletions(-)
 create mode 100644 lib/puppet/faces/help/action.erb
 create mode 100644 lib/puppet/faces/help/face.erb
 create mode 100644 lib/puppet/faces/help/global.erb

diff --git a/lib/puppet/faces/help.rb b/lib/puppet/faces/help.rb
index 17ce3ec6b..c5f4b0fe6 100644
--- a/lib/puppet/faces/help.rb
+++ b/lib/puppet/faces/help.rb
@@ -1,9 +1,9 @@
 require 'puppet/faces'
 require 'puppet/util/command_line'
+require 'pathname'
+require 'erb'
 
 Puppet::Faces.define(:help, '0.0.1') do
-  HelpSummaryFormat = '  %-18s  %s'
-
   summary "Displays help about puppet subcommands"
 
   action(:help) do
@@ -38,54 +38,22 @@ Puppet::Faces.define(:help, '0.0.1') do
       face   = facename ? Puppet::Faces[facename.to_sym, version] : nil
       action = (face and actionname) ? face.get_action(actionname.to_sym) : nil
 
-      # Finally, build up the help text.  Maybe ERB would have been nicer
-      # after all.  Oh, well. --daniel 2011-04-11
-      message = []
-      if args.length == 0 then
-        message << "Use: puppet <subcommand> [options] <action> [options]"
-        message << ""
-        message << "Available subcommands, from Puppet Faces:"
-        Puppet::Faces.faces.sort.each do |name|
-          face = Puppet::Faces[name, :current]
-          message << format(HelpSummaryFormat, face.name, face.summary)
-        end
-
-        unless legacy_applications.empty? then # great victory when this is true!
-          message << ""
-          message << "Available applications, soon to be ported to Faces:"
-          legacy_applications.each do |appname|
-            summary = horribly_extract_summary_from appname
-            message << format(HelpSummaryFormat, appname, summary)
-          end
-        end
-
-        message << ""
-        message << <<EOT.split("\n")
-See 'puppet help <subcommand> <action>' for help on a specific subcommand action.
-See 'puppet help <subcommand>' for help on a specific subcommand.
-See 'puppet man  <subcommand>' for the full man page.
-Puppet v#{Puppet::PUPPETVERSION}
-EOT
-      elsif args.length == 1 then
-        message << "Use: puppet #{face.name} [options] <action> [options]"
-        message << ""
+      template = case args.length
+                 when 0 then erb_template 'global.erb'
+                 when 1 then erb_template 'face.erb'
+                 when 2 then erb_template 'action.erb'
+                 else
+                   fail ArgumentError, "Too many arguments to help action"
+                 end
 
-        message << "Available actions:"
-        face.actions.each do |actionname|
-          action = face.get_action(actionname)
-          message << format(HelpSummaryFormat, action.name, action.summary)
-        end
-
-      elsif args.length == 2
-        "REVISIT: gotta write this code."
-      else
-        raise ArgumentError, "help only takes two arguments, a face name and an action"
-      end
-
-      message
+      return ERB.new(template, nil, '%').result(binding)
     end
   end
 
+  def erb_template(name)
+    (Pathname(__FILE__).dirname + "help" + name).read
+  end
+
   def legacy_applications
     # The list of applications, less those that are duplicated as a face.
     Puppet::Util::CommandLine.available_subcommands.reject do |appname|
diff --git a/lib/puppet/faces/help/action.erb b/lib/puppet/faces/help/action.erb
new file mode 100644
index 000000000..eaf131464
--- /dev/null
+++ b/lib/puppet/faces/help/action.erb
@@ -0,0 +1,3 @@
+Use: puppet <%= face.name %> [options] <%= action.name %> [options]
+
+Summary: <%= action.summary %>
diff --git a/lib/puppet/faces/help/face.erb b/lib/puppet/faces/help/face.erb
new file mode 100644
index 000000000..efe5fd809
--- /dev/null
+++ b/lib/puppet/faces/help/face.erb
@@ -0,0 +1,7 @@
+Use: puppet <%= face.name %> [options] <action> [options]
+
+Available actions:
+% face.actions.each do |actionname|
+%   action = face.get_action(actionname)
+  <%= action.name.to_s.ljust(16) %>  <%= action.summary %>
+% end
diff --git a/lib/puppet/faces/help/global.erb b/lib/puppet/faces/help/global.erb
new file mode 100644
index 000000000..e123367a2
--- /dev/null
+++ b/lib/puppet/faces/help/global.erb
@@ -0,0 +1,20 @@
+puppet <subcommand> [options] <action> [options]
+
+Available subcommands, from Puppet Faces:
+% Puppet::Faces.faces.sort.each do |name|
+%   face = Puppet::Faces[name, :current]
+  <%= face.name.to_s.ljust(16) %>  <%= face.summary %>
+% end
+
+% unless legacy_applications.empty? then # great victory when this is true!
+Available applications, soon to be ported to Faces:
+%   legacy_applications.each do |appname|
+%     summary = horribly_extract_summary_from appname
+  <%= appname.to_s.ljust(16) %>  <%= summary %>
+%   end
+% end
+
+See 'puppet help <subcommand> <action>' for help on a specific subcommand action.
+See 'puppet help <subcommand>' for help on a specific subcommand.
+See 'puppet man  <subcommand>' for the full man page.
+Puppet v<%= Puppet::PUPPETVERSION %>
diff --git a/spec/unit/faces/help_spec.rb b/spec/unit/faces/help_spec.rb
index 61f1947f3..1399abfef 100644
--- a/spec/unit/faces/help_spec.rb
+++ b/spec/unit/faces/help_spec.rb
@@ -100,6 +100,4 @@ describe Puppet::Faces[:help, '0.0.1'] do
       end
     end
   end
-
-  
 end
-- 
cgit 


From 217c1561a86a76b9570bcc4ab0db31eb25d120fa Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Mon, 11 Apr 2011 17:04:42 -0700
Subject: (#6962) Report the template filename for help render errors.

We now set the filename attribute of the ERB instance, which results in any
error messages showing up with the right attribution: to the file they are
defined in, rather than just '(erb)'.

Paired-With: Matt Robinson <matt@puppetlabs.com>
---
 lib/puppet/faces/help.rb | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/lib/puppet/faces/help.rb b/lib/puppet/faces/help.rb
index c5f4b0fe6..26f839735 100644
--- a/lib/puppet/faces/help.rb
+++ b/lib/puppet/faces/help.rb
@@ -39,19 +39,24 @@ Puppet::Faces.define(:help, '0.0.1') do
       action = (face and actionname) ? face.get_action(actionname.to_sym) : nil
 
       template = case args.length
-                 when 0 then erb_template 'global.erb'
-                 when 1 then erb_template 'face.erb'
-                 when 2 then erb_template 'action.erb'
+                 when 0 then erb 'global.erb'
+                 when 1 then erb 'face.erb'
+                 when 2 then erb 'action.erb'
                  else
                    fail ArgumentError, "Too many arguments to help action"
                  end
 
-      return ERB.new(template, nil, '%').result(binding)
+      # Run the ERB template in our current binding, including all the local
+      # variables we established just above. --daniel 2011-04-11
+      return template.result(binding)
     end
   end
 
-  def erb_template(name)
-    (Pathname(__FILE__).dirname + "help" + name).read
+  def erb(name)
+    template = (Pathname(__FILE__).dirname + "help" + name)
+    erb = ERB.new(template.read, nil, '%')
+    erb.filename = template.to_s
+    return erb
   end
 
   def legacy_applications
-- 
cgit 


From 648e3c0dd0fb671b01e54cc0bca202bafc5ae934 Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Mon, 11 Apr 2011 17:11:25 -0700
Subject: (#6962) Better argument checking for help.

We used to blink and miss the fact that we failed to load an action or face in
the past; now we test for that, and fail with a clear error message when the
user asks for something we can't deliver.

Additionally, fix a couple of tests that were silently broken because they
passed the wrong arguments, but still got some output.

Paired-With: Matt Robinson <matt@puppetlabs.com>
---
 lib/puppet/faces/help.rb     | 20 +++++++++++++-------
 spec/unit/faces/help_spec.rb |  7 ++++---
 2 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/lib/puppet/faces/help.rb b/lib/puppet/faces/help.rb
index 26f839735..29a4a62f7 100644
--- a/lib/puppet/faces/help.rb
+++ b/lib/puppet/faces/help.rb
@@ -38,13 +38,19 @@ Puppet::Faces.define(:help, '0.0.1') do
       face   = facename ? Puppet::Faces[facename.to_sym, version] : nil
       action = (face and actionname) ? face.get_action(actionname.to_sym) : nil
 
-      template = case args.length
-                 when 0 then erb 'global.erb'
-                 when 1 then erb 'face.erb'
-                 when 2 then erb 'action.erb'
-                 else
-                   fail ArgumentError, "Too many arguments to help action"
-                 end
+      case args.length
+      when 0 then
+        template = erb 'global.erb'
+      when 1 then
+        face or fail ArgumentError, "Unable to load face #{facename}"
+        template = erb 'face.erb'
+      when 2 then
+        face or fail ArgumentError, "Unable to load face #{facename}"
+        action or fail ArgumentError, "Unable to load action #{actionname} from #{face}"
+        template = erb 'action.erb'
+      else
+        fail ArgumentError, "Too many arguments to help action"
+      end
 
       # Run the ERB template in our current binding, including all the local
       # variables we established just above. --daniel 2011-04-11
diff --git a/spec/unit/faces/help_spec.rb b/spec/unit/faces/help_spec.rb
index 1399abfef..aa811c4b3 100644
--- a/spec/unit/faces/help_spec.rb
+++ b/spec/unit/faces/help_spec.rb
@@ -28,8 +28,8 @@ describe Puppet::Faces[:help, '0.0.1'] do
   end
 
   it "should treat :current and 'current' identically" do
-    subject.help(:help, :current).should ==
-      subject.help(:help, 'current')
+    subject.help(:help, :version => :current).should ==
+      subject.help(:help, :version => 'current')
   end
 
   it "should complain when the request version of a face is missing" do
@@ -39,7 +39,8 @@ describe Puppet::Faces[:help, '0.0.1'] do
 
   it "should find a face by version" do
     face = Puppet::Faces[:huzzah, :current]
-    subject.help(:huzzah, face.version).should == subject.help(:huzzah, :current)
+    subject.help(:huzzah, :version => face.version).
+      should == subject.help(:huzzah, :version => :current)
   end
 
   context "when listing subcommands" do
-- 
cgit 


From acbbd52e0e876afa62f9c6098f0f40504b993dce Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Mon, 11 Apr 2011 17:24:38 -0700
Subject: (#6962) Clean up testing further.

This refactors common code in the tests out, and takes advantage of the
implicit and explicit subject support in rspec2 to make the testing more
expressive and more efficient.

Paired-With: Matt Robinson <matt@puppetlabs.com>
---
 spec/unit/faces/help_spec.rb | 36 +++++++++++-------------------------
 1 file changed, 11 insertions(+), 25 deletions(-)

diff --git a/spec/unit/faces/help_spec.rb b/spec/unit/faces/help_spec.rb
index aa811c4b3..f2aeb44e9 100644
--- a/spec/unit/faces/help_spec.rb
+++ b/spec/unit/faces/help_spec.rb
@@ -59,46 +59,32 @@ describe Puppet::Faces[:help, '0.0.1'] do
       face = Puppet::Faces[name, :current]
       summary = face.summary
 
-      it { should have_matching_element %r{ #{name} } }
-      it { should have_matching_element %r{ #{name} +#{summary}} } if summary
+      it { should =~ %r{ #{name} } }
+      it { should =~ %r{ #{name} +#{summary}} } if summary
     end
 
-    Puppet::Util::CommandLine.available_subcommands do |name|
-      it { should have_matching_element %r{ #{name} } }
+    Puppet::Faces[:help, :current].legacy_applications.each do |appname|
+      it { should =~ %r{ #{appname} } }
+
+      summary = Puppet::Faces[:help, :current].horribly_extract_summary_from(appname)
+      summary and it { should =~ %r{ #{summary}\b} }
     end
   end
 
-  context "when listing legacy applications" do
-    let :help do Puppet::Faces[:help, :current] end
+  context "#legacy_applications" do
+    subject { Puppet::Faces[:help, :current].legacy_applications }
 
     # If we don't, these tests are ... less than useful, because they assume
     # it.  When this breaks you should consider ditching the entire feature
     # and tests, but if not work out how to fake one. --daniel 2011-04-11
-    it "should have at least one legacy application" do
-      help.legacy_applications.should have_at_least(1).item
-    end
+    it { should have_at_least(1).item }
 
     # Meh.  This is nasty, but we can't control the other list; the specific
     # bug that caused these to be listed is annoyingly subtle and has a nasty
     # fix, so better to have a "fail if you do something daft" trigger in
     # place here, I think. --daniel 2011-04-11
     %w{faces_base indirection_base}.each do |name|
-      it "should not list the #{name} application" do
-        help.legacy_applications.should_not include name
-      end
-    end
-
-    Puppet::Faces[:help, :current].legacy_applications.each do |appname|
-      it "should list #{appname} in the help output" do
-        help.help.should have_matching_element %r{ #{appname} }
-      end
-
-      summary = Puppet::Faces[:help, :current].horribly_extract_summary_from(appname)
-      if summary then
-        it "should display the summary of #{appname}" do
-          help.help.should have_matching_element %r{ #{summary}\b}
-        end
-      end
+      it { should_not include name }
     end
   end
 end
-- 
cgit 


From fe6d59528d18e9aa989f48d364868a5a105017a5 Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Mon, 11 Apr 2011 22:04:28 -0700
Subject: (#6962) Integrate legacy subcommands into the help face.

Previously we would only emit help for a face; now we fully integrate legacy
subcommands in the sense that asking for face-level help will emit their
entire help text.

Asking for any action subsequent will raise an error: this is an annoyingly
inconsistent behaviour, but there isn't a saner definition of the change.

Reviewed-By: Matt Robinson <matt@puppetlabs.com>
---
 lib/puppet/faces/help.rb     | 12 ++++++++++--
 spec/unit/faces/help_spec.rb | 22 ++++++++++++++++++++++
 2 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/lib/puppet/faces/help.rb b/lib/puppet/faces/help.rb
index 29a4a62f7..1d8abe20e 100644
--- a/lib/puppet/faces/help.rb
+++ b/lib/puppet/faces/help.rb
@@ -35,8 +35,16 @@ Puppet::Faces.define(:help, '0.0.1') do
 
       # Name those parameters...
       facename, actionname = args
-      face   = facename ? Puppet::Faces[facename.to_sym, version] : nil
-      action = (face and actionname) ? face.get_action(actionname.to_sym) : nil
+
+      if facename then
+        if legacy_applications.include? facename then
+          actionname and raise ArgumentError, "Legacy subcommands don't take actions"
+          return Puppet::Application[facename].help
+        else
+          face = Puppet::Faces[facename.to_sym, version]
+          actionname and action = face.get_action(actionname.to_sym)
+        end
+      end
 
       case args.length
       when 0 then
diff --git a/spec/unit/faces/help_spec.rb b/spec/unit/faces/help_spec.rb
index f2aeb44e9..cd74a5bf1 100644
--- a/spec/unit/faces/help_spec.rb
+++ b/spec/unit/faces/help_spec.rb
@@ -87,4 +87,26 @@ describe Puppet::Faces[:help, '0.0.1'] do
       it { should_not include name }
     end
   end
+
+  context "help for legacy applications" do
+    subject { Puppet::Faces[:help, :current] }
+    let :appname do subject.legacy_applications.first end
+
+    # This test is purposely generic, so that as we eliminate legacy commands
+    # we don't get into a loop where we either test a face-based replacement
+    # and fail to notice breakage, or where we have to constantly rewrite this
+    # test and all. --daniel 2011-04-11
+    it "should return the legacy help when given the subcommand" do
+      help = subject.help(appname)
+      help.should =~ /puppet-#{appname}/
+      %w{SYNOPSIS USAGE DESCRIPTION OPTIONS COPYRIGHT}.each do |heading|
+        help.should =~ /^#{heading}$/
+      end
+    end
+
+    it "should fail when asked for an action on a legacy command" do
+      expect { subject.help(appname, :whatever) }.
+        to raise_error ArgumentError, /Legacy subcommands don't take actions/
+    end
+  end
 end
-- 
cgit 


From 9496067f5644972def823cf8c3318aca137b86fe Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Mon, 11 Apr 2011 22:25:58 -0700
Subject: maint: avoid making temporary dirs during testing.

We used to create temporary directories to have some support files on disk
during testing of faces; we don't really need that most of the time, and this
updates a test to reflect that reality.

Reviewed-By: Matt Robinson <matt@puppetlabs.com>
---
 spec/lib/puppet/faces/basetest.rb        |  1 +
 spec/unit/application/faces_base_spec.rb | 14 +-------------
 2 files changed, 2 insertions(+), 13 deletions(-)
 create mode 100644 spec/lib/puppet/faces/basetest.rb

diff --git a/spec/lib/puppet/faces/basetest.rb b/spec/lib/puppet/faces/basetest.rb
new file mode 100644
index 000000000..d20c52b97
--- /dev/null
+++ b/spec/lib/puppet/faces/basetest.rb
@@ -0,0 +1 @@
+Puppet::Faces.define(:basetest, '0.0.1')
diff --git a/spec/unit/application/faces_base_spec.rb b/spec/unit/application/faces_base_spec.rb
index b7a11ad56..a6df55aef 100755
--- a/spec/unit/application/faces_base_spec.rb
+++ b/spec/unit/application/faces_base_spec.rb
@@ -1,4 +1,4 @@
-#!/usr/bin/env ruby
+#!/usr/bin/env rspec
 
 require 'spec_helper'
 require 'puppet/application/faces_base'
@@ -9,13 +9,6 @@ end
 
 describe Puppet::Application::FacesBase do
   before :all do
-    @dir = Dir.mktmpdir
-    $LOAD_PATH.push(@dir)
-    FileUtils.mkdir_p(File.join @dir, 'puppet', 'faces')
-    File.open(File.join(@dir, 'puppet', 'faces', 'basetest.rb'), 'w') do |f|
-      f.puts "Puppet::Faces.define(:basetest, '0.0.1')"
-    end
-
     Puppet::Faces.define(:basetest, '0.0.1') do
       option("--[no-]boolean")
       option("--mandatory MANDATORY")
@@ -28,11 +21,6 @@ describe Puppet::Application::FacesBase do
     end
   end
 
-  after :all do
-    FileUtils.remove_entry_secure @dir
-    $LOAD_PATH.pop
-  end
-
   let :app do
     app = Puppet::Application::FacesBase::Basetest.new
     app.stubs(:exit)
-- 
cgit 


From 826d5dff531eb624fef91a7298932e9ec5a46231 Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Mon, 11 Apr 2011 23:09:18 -0700
Subject: (#6962) delegate global usage to the help face.

The global --help handler (also invoked when puppet is run without any other
command line options at all) used to spit out a brief and generally not so
helpful message.

Now that we have a help face that can provide the same information in a much
more user-friendly form, we should delegate the function to that when
required.

Reviewed-By: Matt Robinson <matt@puppetlabs.com>
---
 lib/puppet/util/command_line.rb     | 23 +++++++++++++----------
 spec/unit/application/cert_spec.rb  | 14 +-------------
 spec/unit/util/command_line_spec.rb |  7 ++++---
 3 files changed, 18 insertions(+), 26 deletions(-)

diff --git a/lib/puppet/util/command_line.rb b/lib/puppet/util/command_line.rb
index 2891030df..674927842 100644
--- a/lib/puppet/util/command_line.rb
+++ b/lib/puppet/util/command_line.rb
@@ -49,26 +49,29 @@ module Puppet
         self.class.available_subcommands
       end
 
-      def usage_message
-        usage = "Usage: puppet command <space separated arguments>"
-        available = "Available commands are: #{available_subcommands.sort.join(', ')}"
-        [usage, available].join("\n")
-      end
-
       def require_application(application)
         require File.join(appdir, application)
       end
 
       def execute
-        if subcommand_name.nil?
-          puts usage_message
-        elsif available_subcommands.include?(subcommand_name) #subcommand
+        if subcommand_name and available_subcommands.include?(subcommand_name) then
           require_application subcommand_name
           app = Puppet::Application.find(subcommand_name).new(self)
           Puppet::Plugins.on_application_initialization(:appliation_object => self)
           app.run
+        elsif execute_external_subcommand then
+          # Logically, we shouldn't get here, but we do, so whatever.  We just
+          # return to the caller.  How strange we are. --daniel 2011-04-11
         else
-          abort "Error: Unknown command #{subcommand_name}.\n#{usage_message}" unless execute_external_subcommand
+          unless subcommand_name.nil? then
+            puts "Error: Unknown Puppet subcommand #{subcommand_name}.\n"
+          end
+
+          # Doing this at the top of the file is natural, but causes puppet.rb
+          # to load too early, which causes things to break.  This is a nasty
+          # thing, found in #7065. --daniel 2011-04-11
+          require 'puppet/faces/help'
+          puts Puppet::Faces[:help, :current].help
         end
       end
 
diff --git a/spec/unit/application/cert_spec.rb b/spec/unit/application/cert_spec.rb
index 5b25ab7b8..a1b5eb19a 100755
--- a/spec/unit/application/cert_spec.rb
+++ b/spec/unit/application/cert_spec.rb
@@ -1,7 +1,5 @@
-#!/usr/bin/env ruby
-
+#!/usr/bin/env rspec
 require 'spec_helper'
-
 require 'puppet/application/cert'
 
 describe Puppet::Application::Cert do
@@ -189,16 +187,6 @@ describe Puppet::Application::Cert do
       @cert_app.ca = @ca
     end
 
-    it "should SystemExit after printing help message" do
-      # Make the help method silent for testing; this is a bit nasty, but we
-      # can't identify a cleaner method.  Help welcome. --daniel 2011-02-22
-      Puppet.features.stubs(:usage?).returns(false)
-      @cert_app.stubs(:puts)
-
-      @cert_app.command_line.stubs(:args).returns([])
-      expect { @cert_app.parse_options }.should raise_error SystemExit
-    end
-
     %w{list revoke generate sign print verify fingerprint}.each do |cmd|
       short = cmd[0,1]
       [cmd, "--#{cmd}", "-#{short}"].each do |option|
diff --git a/spec/unit/util/command_line_spec.rb b/spec/unit/util/command_line_spec.rb
index c5c6e5f3e..6cf90475b 100755
--- a/spec/unit/util/command_line_spec.rb
+++ b/spec/unit/util/command_line_spec.rb
@@ -99,10 +99,11 @@ describe Puppet::Util::CommandLine do
         Puppet::Util.expects(:which).with('puppet-whatever').returns(nil)
         commandline.expects(:system).never
 
-        commandline.expects(:usage_message).returns("the usage message")
-        commandline.expects(:abort).with{|x| x =~ /the usage message/}.raises("stubbed abort")
+        text = Puppet::Faces[:help, :current].help
+        commandline.expects(:puts).with { |x| x =~ /Unknown Puppet subcommand/ }
+        commandline.expects(:puts).with text
 
-        lambda{ commandline.execute }.should raise_error('stubbed abort')
+        commandline.execute
       end
     end
   end
-- 
cgit 


From 7899462b2ec1114907844907c77b1742193467b9 Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Mon, 11 Apr 2011 23:11:35 -0700
Subject: maint: whitespace cleanup for puppet/util/command_line.

This brings bracket spacing in line with our usual coding conventions.

Reviewed-By: Matt Robinson <matt@puppetlabs.com>
---
 lib/puppet/util/command_line.rb | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/lib/puppet/util/command_line.rb b/lib/puppet/util/command_line.rb
index 674927842..fa462ee2d 100644
--- a/lib/puppet/util/command_line.rb
+++ b/lib/puppet/util/command_line.rb
@@ -17,12 +17,12 @@ module Puppet
         'master'     => 'puppetmasterd'
       )
 
-      def initialize( zero = $0, argv = ARGV, stdin = STDIN )
+      def initialize(zero = $0, argv = ARGV, stdin = STDIN)
         @zero  = zero
         @argv  = argv.dup
         @stdin = stdin
 
-        @subcommand_name, @args = subcommand_and_args( @zero, @argv, @stdin )
+        @subcommand_name, @args = subcommand_and_args(@zero, @argv, @stdin)
         Puppet::Plugins.on_commandline_initialization(:command_line_object => self)
       end
 
@@ -79,10 +79,10 @@ module Puppet
         external_command = "puppet-#{subcommand_name}"
 
         require 'puppet/util'
-        path_to_subcommand = Puppet::Util.which( external_command )
+        path_to_subcommand = Puppet::Util.which(external_command)
         return false unless path_to_subcommand
 
-        system( path_to_subcommand, *args )
+        system(path_to_subcommand, *args)
         true
       end
 
@@ -92,7 +92,7 @@ module Puppet
 
       private
 
-      def subcommand_and_args( zero, argv, stdin )
+      def subcommand_and_args(zero, argv, stdin)
         zero = File.basename(zero, '.rb')
 
         if zero == 'puppet'
-- 
cgit 


From 7b4d9367b391f75983868046d30928ebc8411f50 Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Tue, 12 Apr 2011 11:45:05 -0700
Subject: (#6962) Move option handling into #parse_options, not #preinit.

Logically, the extra work around option parsing for faces belongs in the
application parse_options method, not hidden in the step before.  This commit
moves that to the right place and fixes the fallout from that strange early
design decision.

Along the way we unify error reporting for invalid options so that all the
code paths result in the same externally detected failures.

Reviewed-By: Matt Robinson <matt@puppetlabs.com>
---
 lib/puppet/application.rb                 | 12 +++----
 lib/puppet/application/faces_base.rb      | 18 +++++++---
 spec/unit/application/certificate_spec.rb |  5 ++-
 spec/unit/application/faces_base_spec.rb  | 60 +++++++++++++++++--------------
 spec/unit/application_spec.rb             | 10 ------
 5 files changed, 55 insertions(+), 50 deletions(-)

diff --git a/lib/puppet/application.rb b/lib/puppet/application.rb
index 4c5a5a967..f3a749786 100644
--- a/lib/puppet/application.rb
+++ b/lib/puppet/application.rb
@@ -359,14 +359,10 @@ class Application
       end
     end
 
-    # scan command line.
-    begin
-      option_parser.parse!(self.command_line.args)
-    rescue OptionParser::ParseError => detail
-      $stderr.puts detail
-      $stderr.puts "Try 'puppet #{command_line.subcommand_name} --help'"
-      exit(1)
-    end
+    # Scan command line.  We just hand any exceptions to our upper levels,
+    # rather than printing help and exiting, so that we can meaningfully
+    # respond with context-sensitive help if we want to. --daniel 2011-04-12
+    option_parser.parse!(self.command_line.args)
   end
 
   def handlearg(opt, arg)
diff --git a/lib/puppet/application/faces_base.rb b/lib/puppet/application/faces_base.rb
index 288b50048..f1b77f285 100644
--- a/lib/puppet/application/faces_base.rb
+++ b/lib/puppet/application/faces_base.rb
@@ -1,5 +1,6 @@
 require 'puppet/application'
 require 'puppet/faces'
+require 'optparse'
 
 class Puppet::Application::FacesBase < Puppet::Application
   should_parse_config
@@ -50,11 +51,13 @@ class Puppet::Application::FacesBase < Puppet::Application
       $stderr.puts "Cancelling Face"
       exit(0)
     end
+  end
 
+  def parse_options
     # We need to parse enough of the command line out early, to identify what
     # the action is, so that we can obtain the full set of options to parse.
 
-    # TODO: These should be configurable versions, through a global
+    # REVISIT: These should be configurable versions, through a global
     # '--version' option, but we don't implement that yet... --daniel 2011-03-29
     @type   = self.class.name.to_s.sub(/.+:/, '').downcase.to_sym
     @face   = Puppet::Faces[@type, :current]
@@ -88,25 +91,30 @@ class Puppet::Application::FacesBase < Puppet::Application
             index += 1          # ...so skip the argument.
           end
         else
-          raise ArgumentError, "Unknown option #{item.sub(/=.*$/, '').inspect}"
+          raise OptionParser::InvalidOption.new(item.sub(/=.*$/, ''))
         end
       else
         action = @face.get_action(item.to_sym)
         if action.nil? then
-          raise ArgumentError, "#{@face} does not have an #{item.inspect} action!"
+          raise OptionParser::InvalidArgument.new("#{@face} does not have an #{item} action")
         end
         @action = action
       end
     end
 
-    @action or raise ArgumentError, "No action given on the command line!"
+    unless @action
+      raise OptionParser::MissingArgument.new("No action given on the command line")
+    end
 
-    # Finally, we can interact with the default option code to build behaviour
+    # Now we can interact with the default option code to build behaviour
     # around the full set of options we now know we support.
     @action.options.each do |option|
       option = @action.get_option(option) # make it the object.
       self.class.option(*option.optparse) # ...and make the CLI parse it.
     end
+
+    # ...and invoke our parent to parse all the command line options.
+    super
   end
 
   def find_global_settings_argument(item)
diff --git a/spec/unit/application/certificate_spec.rb b/spec/unit/application/certificate_spec.rb
index 6153d9538..27d6ac81b 100755
--- a/spec/unit/application/certificate_spec.rb
+++ b/spec/unit/application/certificate_spec.rb
@@ -6,12 +6,15 @@ describe Puppet::Application::Certificate do
     # so is this actually a valuable test? --daniel 2011-04-07
     subject.command_line.stubs(:args).returns %w{list}
     subject.preinit
+    subject.parse_options
     subject.should respond_to(:handle_ca_location)
   end
 
   it "should accept the ca-location option" do
     subject.command_line.stubs(:args).returns %w{--ca-location local list}
-    subject.preinit and subject.parse_options and subject.setup
+    subject.preinit
+    subject.parse_options
+    subject.setup
     subject.arguments.should == [{ :ca_location => "local" }]
   end
 end
diff --git a/spec/unit/application/faces_base_spec.rb b/spec/unit/application/faces_base_spec.rb
index a6df55aef..18bd30295 100755
--- a/spec/unit/application/faces_base_spec.rb
+++ b/spec/unit/application/faces_base_spec.rb
@@ -23,9 +23,7 @@ describe Puppet::Application::FacesBase do
 
   let :app do
     app = Puppet::Application::FacesBase::Basetest.new
-    app.stubs(:exit)
-    app.stubs(:puts)
-    app.command_line.stubs(:subcommand_name).returns 'subcommand'
+    app.command_line.stubs(:subcommand_name).returns('subcommand')
     Puppet::Util::Log.stubs(:newdestination)
     app
   end
@@ -39,7 +37,7 @@ describe Puppet::Application::FacesBase do
     end
   end
 
-  describe "#preinit" do
+  describe "#parse_options" do
     before :each do
       app.command_line.stubs(:args).returns %w{}
     end
@@ -56,6 +54,7 @@ describe Puppet::Application::FacesBase do
           Signal.stubs(:trap)
           app.command_line.stubs(:args).returns %w{foo}
           app.preinit
+          app.parse_options
         end
 
         it "should set the faces based on the type" do
@@ -73,54 +72,54 @@ describe Puppet::Application::FacesBase do
       end
 
       it "should fail if no action is given" do
-        expect { app.preinit }.
-          should raise_error ArgumentError, /No action given/
+        expect { app.preinit; app.parse_options }.
+          to raise_error OptionParser::MissingArgument, /No action given/
       end
 
       it "should report a sensible error when options with = fail" do
         app.command_line.stubs(:args).returns %w{--action=bar foo}
-        expect { app.preinit }.
-          should raise_error ArgumentError, /Unknown option "--action"/
+        expect { app.preinit; app.parse_options }.
+          to raise_error OptionParser::InvalidOption, /invalid option: --action/
       end
 
       it "should fail if an action option is before the action" do
         app.command_line.stubs(:args).returns %w{--action foo}
-        expect { app.preinit }.
-          should raise_error ArgumentError, /Unknown option "--action"/
+        expect { app.preinit; app.parse_options }.
+          to raise_error OptionParser::InvalidOption, /invalid option: --action/
       end
 
       it "should fail if an unknown option is before the action" do
         app.command_line.stubs(:args).returns %w{--bar foo}
-        expect { app.preinit }.
-          should raise_error ArgumentError, /Unknown option "--bar"/
+        expect { app.preinit; app.parse_options }.
+          to raise_error OptionParser::InvalidOption, /invalid option: --bar/
       end
 
-      it "should not fail if an unknown option is after the action" do
+      it "should fail if an unknown option is after the action" do
         app.command_line.stubs(:args).returns %w{foo --bar}
-        app.preinit
-        app.action.name.should == :foo
-        app.face.should_not be_option :bar
-        app.action.should_not be_option :bar
+        expect { app.preinit; app.parse_options }.
+          to raise_error OptionParser::InvalidOption, /invalid option: --bar/
       end
 
       it "should accept --bar as an argument to a mandatory option after action" do
         app.command_line.stubs(:args).returns %w{foo --mandatory --bar}
-        app.preinit and app.parse_options
+        app.preinit
+        app.parse_options
         app.action.name.should == :foo
         app.options.should == { :mandatory => "--bar" }
       end
 
       it "should accept --bar as an argument to a mandatory option before action" do
         app.command_line.stubs(:args).returns %w{--mandatory --bar foo}
-        app.preinit and app.parse_options
+        app.preinit
+        app.parse_options
         app.action.name.should == :foo
         app.options.should == { :mandatory => "--bar" }
       end
 
       it "should not skip when --foo=bar is given" do
         app.command_line.stubs(:args).returns %w{--mandatory=bar --bar foo}
-        expect { app.preinit }.
-          should raise_error ArgumentError, /Unknown option "--bar"/
+        expect { app.preinit; app.parse_options }.
+          to raise_error OptionParser::InvalidOption, /invalid option: --bar/
       end
 
       { "boolean options before" => %w{--trace foo},
@@ -128,7 +127,8 @@ describe Puppet::Application::FacesBase do
       }.each do |name, args|
         it "should accept global boolean settings #{name} the action" do
           app.command_line.stubs(:args).returns args
-          app.preinit && app.parse_options
+          app.preinit
+          app.parse_options
           Puppet[:trace].should be_true
         end
       end
@@ -138,7 +138,8 @@ describe Puppet::Application::FacesBase do
       }.each do |name, args|
         it "should accept global settings with arguments #{name} the action" do
           app.command_line.stubs(:args).returns args
-          app.preinit && app.parse_options
+          app.preinit
+          app.parse_options
           Puppet[:syslogfacility].should == "user1"
         end
       end
@@ -148,19 +149,25 @@ describe Puppet::Application::FacesBase do
   describe "#setup" do
     it "should remove the action name from the arguments" do
       app.command_line.stubs(:args).returns %w{--mandatory --bar foo}
-      app.preinit and app.parse_options and app.setup
+      app.preinit
+      app.parse_options
+      app.setup
       app.arguments.should == [{ :mandatory => "--bar" }]
     end
 
     it "should pass positional arguments" do
       app.command_line.stubs(:args).returns %w{--mandatory --bar foo bar baz quux}
-      app.preinit and app.parse_options and app.setup
+      app.preinit
+      app.parse_options
+      app.setup
       app.arguments.should == ['bar', 'baz', 'quux', { :mandatory => "--bar" }]
     end
   end
 
   describe "#main" do
-    before do
+    before :each do
+      app.expects(:exit).with(0)
+
       app.face      = Puppet::Faces[:basetest, '0.0.1']
       app.action    = app.face.get_action(:foo)
       app.format    = :pson
@@ -174,6 +181,7 @@ describe Puppet::Application::FacesBase do
 
     it "should use its render method to render any result" do
       app.expects(:render).with(app.arguments.length + 1)
+      app.stubs(:puts)          # meh.  Don't print nil, thanks. --daniel 2011-04-12
       app.main
     end
   end
diff --git a/spec/unit/application_spec.rb b/spec/unit/application_spec.rb
index 740b76f62..a1a46c814 100755
--- a/spec/unit/application_spec.rb
+++ b/spec/unit/application_spec.rb
@@ -357,16 +357,6 @@ describe Puppet::Application do
       end
 
     end
-
-    it "should exit if OptionParser raises an error" do
-      $stderr.stubs(:puts)
-      OptionParser.any_instance.stubs(:parse!).raises(OptionParser::ParseError.new("blah blah"))
-
-      @app.expects(:exit)
-
-      lambda { @app.parse_options }.should_not raise_error
-    end
-
   end
 
   describe "when calling default setup" do
-- 
cgit