(#6749) string cli base: implement preinit CLI parsing

In order to identify the full set of options we need to know the action that
is being invoked; that actually requires a pre-processing step to identify
that out of the global options.

Notably, our spec is that options can be to the right of their declaration
point, but not to the left: that means that we can now extract the full set of
options, and interact with the standard Puppet option handling code, resulting
in at least vaguely saner behaviour...

Reviewed-By: Pieter van de Bruggen <pieter@puppetlabs.com>

4 files changed, 140 insertions, 49 deletions

diff --git a/lib/puppet/application/string_base.rb b/lib/puppet/application/string_base.rb
index ffd49e8c0..1169a45a4 100644
--- a/lib/puppet/application/string_base.rb
+++ b/lib/puppet/application/string_base.rb
@@ -5,14 +5,6 @@ class Puppet::Application::StringBase < Puppet::Application
   should_parse_config
   run_mode :agent
 
-  def preinit
-    super
-    trap(:INT) do
-      $stderr.puts "Cancelling String"
-      exit(0)
-    end
-  end
-
   option("--debug", "-d") do |arg|
     Puppet::Util::Log.level = :debug
   end
@@ -32,7 +24,7 @@ class Puppet::Application::StringBase < Puppet::Application
   end
 
 
-  attr_accessor :string, :type, :verb, :arguments, :format
+  attr_accessor :string, :action, :type, :arguments, :format
   attr_writer :exit_code
 
   # This allows you to set the exit code if you don't want to just exit
@@ -41,14 +33,6 @@ class Puppet::Application::StringBase < Puppet::Application
     @exit_code || 0
   end
 
-  def main
-    # Call the method associated with the provided action (e.g., 'find').
-    if result = string.send(verb, *arguments)
-      puts render(result)
-    end
-    exit(exit_code)
-  end
-
   # Override this if you need custom rendering.
   def render(result)
     render_method = Puppet::Network::FormatHandler.format(format).render_method
@@ -61,16 +45,14 @@ class Puppet::Application::StringBase < Puppet::Application
   end
 
   def preinit
+    super
+    trap(:INT) do
+      $stderr.puts "Cancelling String"
+      exit(0)
+    end
+
     # 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.
-    #
-    # This requires a partial parse first, and removing the options that we
-    # understood, then identification of the next item, then another round of
-    # the same until we have the string and action all set. --daniel 2011-03-29
-    #
-    # NOTE: We can't use the Puppet::Application implementation of option
-    # parsing because it is (*ahem*) going to puts on $stderr and exit when it
-    # hits a parse problem, not actually let us reuse stuff. --daniel 2011-03-29
 
     # TODO: These should be configurable versions, through a global
     # '--version' option, but we don't implement that yet... --daniel 2011-03-29
@@ -78,19 +60,42 @@ class Puppet::Application::StringBase < Puppet::Application
     @string = Puppet::String[@type, :current]
     @format = @string.default_format
 
-    # Now, collect the global and string options and parse the command line.
-    begin
-      @string.options.inject OptionParser.new do |options, option|
-        option = @string.get_option option # turn it into the object, bleh
-        options.on(*option.to_optparse) do |value|
-          puts "REVISIT: do something with #{value.inspect}"
+    # Now, walk the command line and identify the action.  We skip over
+    # arguments based on introspecting the action and all, and find the first
+    # non-option word to use as the action.
+    action = nil
+    cli    = command_line.args.dup # we destroy this copy, but...
+    while @action.nil? and not cli.empty? do
+      item = cli.shift
+      if item =~ /^-/ then
+        option = @string.options.find { |a| item =~ /^-+#{a}\b/ }
+        if option then
+          if @string.get_option(option).takes_argument? then
+            # We don't validate if the argument is optional or mandatory,
+            # because it doesn't matter here.  We just assume that errors will
+            # be caught later. --daniel 2011-03-30
+            cli.shift unless cli.first =~ /^-/
+          end
+        else
+          raise ArgumentError, "Unknown option #{item.sub(/=.*$/, '').inspect}"
         end
-      end.parse! command_line.args.dup
-    rescue OptionParser::InvalidOption => e
-      puts e.inspect            # ...and ignore??
+      else
+        action = @string.get_action(item.to_sym)
+        if action.nil? then
+          raise ArgumentError, "#{@string} does not have an #{item.inspect} action!"
+        end
+        @action = action
+      end
     end
 
-    fail "REVISIT: Finish this code, eh..."
+    @action or raise ArgumentError, "No action given on the command line!"
+
+    # Finally, 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
   end
 
   def setup
@@ -100,18 +105,21 @@ class Puppet::Application::StringBase < Puppet::Application
     # action to read in the options.  This replaces the older model where we
     # would invoke the action with options set as global state in the
     # interface object.  --daniel 2011-03-28
-    @verb = command_line.args.shift
     @arguments = Array(command_line.args) << options
     validate
   end
 
+
+  def main
+    # Call the method associated with the provided action (e.g., 'find').
+    if result = @string.send(@action.name, *arguments)
+      puts render(result)
+    end
+    exit(exit_code)
+  end
   def validate
-    unless verb
+    unless @action
       raise "You must specify #{string.actions.join(", ")} as a verb; 'save' probably does not work right now"
     end
-
-    unless string.action?(verb)
-      raise "Command '#{verb}' not found for #{type}"
-    end
   end
 end
diff --git a/lib/puppet/string/option.rb b/lib/puppet/string/option.rb
index 70d62a01f..e7b6f187c 100644
--- a/lib/puppet/string/option.rb
+++ b/lib/puppet/string/option.rb
@@ -2,6 +2,7 @@ class Puppet::String::Option
   attr_reader   :parent
   attr_reader   :name
   attr_reader   :aliases
+  attr_reader   :optparse
   attr_accessor :desc
 
   def takes_argument?
diff --git a/spec/unit/application/string_base_spec.rb b/spec/unit/application/string_base_spec.rb
index 65cadb8fd..1072d9be9 100755
--- a/spec/unit/application/string_base_spec.rb
+++ b/spec/unit/application/string_base_spec.rb
@@ -23,11 +23,72 @@ describe Puppet::Application::StringBase do
     $LOAD_PATH.pop
   end
 
-  before do
-    @app = Puppet::Application::StringBase::Basetest.new
-    @app.stubs(:exit)
-    @app.stubs(:puts)
+  let :app do
+    app = Puppet::Application::StringBase::Basetest.new
+    app.stubs(:exit)
+    app.stubs(:puts)
+    app.command_line.stubs(:subcommand_name).returns 'subcommand'
+    app.command_line.stubs(:args).returns []
     Puppet::Util::Log.stubs(:newdestination)
+    app
+  end
+
+  describe "#preinit" do
+    before :each do
+      app.command_line.stubs(:args).returns %w{}
+    end
+
+    it "should set the string based on the type"
+    it "should set the format based on the string default"
+
+    describe "parsing the command line" do
+      before :all do
+        Puppet::String[:basetest, '0.0.1'].action :foo do
+          option "--foo"
+          invoke do |options|
+            options
+          end
+        end
+      end
+
+      it "should find the action" do
+        app.command_line.stubs(:args).returns %w{foo}
+        app.preinit
+        app.action.should be
+        app.action.name.should == :foo
+      end
+
+      it "should fail if no action is given" do
+        expect { app.preinit }.
+          should raise_error ArgumentError, /No action given/
+      end
+
+      it "should report a sensible error when options with = fail" do
+        app.command_line.stubs(:args).returns %w{--foo=bar foo}
+        expect { app.preinit }.
+          should raise_error ArgumentError, /Unknown option "--foo"/
+      end
+
+      it "should fail if an action option is before the action" do
+        app.command_line.stubs(:args).returns %w{--foo foo}
+        expect { app.preinit }.
+          should raise_error ArgumentError, /Unknown option "--foo"/
+      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"/
+      end
+
+      it "should not 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.string.should_not be_option :bar
+        app.action.should_not be_option :bar
+      end
+    end
   end
 
   describe "when calling main" do
@@ -39,8 +100,7 @@ describe Puppet::Application::StringBase do
 
     it "should send the specified verb and name to the string" do
       @app.string.expects(:find).with("myname", "myarg")
-
-      @app.main
+      app.main
     end
 
     it "should use its render method to render any result"
@@ -50,8 +110,8 @@ describe Puppet::Application::StringBase do
 
   describe "during setup" do
     before do
-      @app.command_line.stubs(:args).returns(["find", "myname", "myarg"])
-      @app.stubs(:validate)
+      app.command_line.stubs(:args).returns(["find", "myname", "myarg"])
+      app.stubs(:validate)
     end
 
     it "should set the verb from the command line arguments" do
diff --git a/spec/unit/string/action_spec.rb b/spec/unit/string/action_spec.rb
index 258ad5aa6..e5fefdbdc 100755
--- a/spec/unit/string/action_spec.rb
+++ b/spec/unit/string/action_spec.rb
@@ -62,6 +62,28 @@ describe Puppet::String::Action do
       string.baz.should  == "the value of foo in baz is '25'"
       string.qux.should  == "the value of foo in baz is '25'"
     end
+
+    context "when calling the Ruby API" do
+      let :string do
+        Puppet::String.new(:ruby_api, '1.0.0') do
+          action :bar do
+            invoke do |options|
+              options
+            end
+          end
+        end
+      end
+
+      it "should work when no options are supplied" do
+        options = string.bar
+        options.should == {}
+      end
+
+      it "should work when options are supplied" do
+        options = string.bar :bar => "beer"
+        options.should == { :bar => "beer" }
+      end
+    end
   end
 
   describe "with action-level options" do