(#6749) implementing option handling in CLI string wrapper

The purpose of this is to adapt the generic option support in our strings to
the command line; we adapt the generic option information to optparse, and
establish our environment early in the process to ensure that we can play nice
with Puppet::Application for the moment.

In the process we ensure that we detect, and report, conflicts in option
naming across the board.  Additionally, when an option is declared with
multiple aliases, we insist that either all, or none, of them take an
argument.

To support this we support introspecting options having an optional argument,
as well as documentation and all.

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

8 files changed, 134 insertions, 47 deletions

diff --git a/lib/puppet/application/indirection_base.rb b/lib/puppet/application/indirection_base.rb
index da61f408d..61cfb435e 100644
--- a/lib/puppet/application/indirection_base.rb
+++ b/lib/puppet/application/indirection_base.rb
@@ -1,15 +1,12 @@
 require 'puppet/application/string_base'
 
 class Puppet::Application::IndirectionBase < Puppet::Application::StringBase
-  option("--terminus TERMINUS") do |arg|
-    @terminus = arg
-  end
-
   attr_accessor :terminus, :indirection
 
   def setup
     super
 
+    # REVISIT: need to implement this in terms of the string options, eh.
     if string.respond_to?(:indirection)
       raise "Could not find data type #{type} for application #{self.class.name}" unless string.indirection
 
diff --git a/lib/puppet/application/string_base.rb b/lib/puppet/application/string_base.rb
index 762fbfda8..ffd49e8c0 100644
--- a/lib/puppet/application/string_base.rb
+++ b/lib/puppet/application/string_base.rb
@@ -60,6 +60,39 @@ class Puppet::Application::StringBase < Puppet::Application
     end
   end
 
+  def preinit
+    # 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
+    @type   = self.class.name.to_s.sub(/.+:/, '').downcase.to_sym
+    @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}"
+        end
+      end.parse! command_line.args.dup
+    rescue OptionParser::InvalidOption => e
+      puts e.inspect            # ...and ignore??
+    end
+
+    fail "REVISIT: Finish this code, eh..."
+  end
+
   def setup
     Puppet::Util::Log.newdestination :console
 
@@ -69,16 +102,6 @@ class Puppet::Application::StringBase < Puppet::Application
     # interface object.  --daniel 2011-03-28
     @verb = command_line.args.shift
     @arguments = Array(command_line.args) << options
-
-    @type = self.class.name.to_s.sub(/.+:/, '').downcase.to_sym
-
-    # TODO: These should be configurable versions.
-    unless Puppet::String.string?(@type, :current)
-      raise "Could not find any version of string '#{@type}'"
-    end
-    @string = Puppet::String[@type, :current]
-    @format ||= @string.default_format
-
     validate
   end
 
diff --git a/lib/puppet/string/action.rb b/lib/puppet/string/action.rb
index 4219aca0a..692e467b4 100644
--- a/lib/puppet/string/action.rb
+++ b/lib/puppet/string/action.rb
@@ -29,13 +29,19 @@ class Puppet::String::Action
   end
 
   def add_option(option)
-    if option? option.name then
-      raise ArgumentError, "#{option.name} duplicates an existing option on #{self}"
-    elsif @string.option? option.name then
-      raise ArgumentError, "#{option.name} duplicates an existing option on #{@string}"
+    option.aliases.each do |name|
+      if conflict = get_option(name) then
+        raise ArgumentError, "Option #{option} conflicts with existing option #{conflict}"
+      elsif conflict = @string.get_option(name) then
+        raise ArgumentError, "Option #{option} conflicts with existing option #{conflict} on #{@string}"
+      end
     end
 
-    @options[option.name] = option
+    option.aliases.each do |name|
+      @options[name] = option
+    end
+
+    option
   end
 
   def option?(name)
diff --git a/lib/puppet/string/action_builder.rb b/lib/puppet/string/action_builder.rb
index fb2a749ae..e76044470 100644
--- a/lib/puppet/string/action_builder.rb
+++ b/lib/puppet/string/action_builder.rb
@@ -23,8 +23,8 @@ class Puppet::String::ActionBuilder
     @action.invoke = block
   end
 
-  def option(name, attrs = {}, &block)
-    option = Puppet::String::OptionBuilder.build(@action, name, attrs, &block)
+  def option(*declaration, &block)
+    option = Puppet::String::OptionBuilder.build(@action, *declaration, &block)
     @action.add_option(option)
   end
 end
diff --git a/lib/puppet/string/indirector.rb b/lib/puppet/string/indirector.rb
index 48ec32680..71b1f3b12 100644
--- a/lib/puppet/string/indirector.rb
+++ b/lib/puppet/string/indirector.rb
@@ -2,6 +2,12 @@ require 'puppet'
 require 'puppet/string'
 
 class Puppet::String::Indirector < Puppet::String
+  warn "REVISIT: Need to redefine this to take arguments again, eh."
+  option "--terminus TERMINUS" do
+    desc "REVISIT: You can select a terminus, which has some bigger effect
+that we should describe in this file somehow."
+  end
+
   def self.indirections
     Puppet::Indirector::Indirection.instances.collect { |t| t.to_s }.sort
   end
diff --git a/lib/puppet/string/option.rb b/lib/puppet/string/option.rb
index bdc3e07c5..26b769c2e 100644
--- a/lib/puppet/string/option.rb
+++ b/lib/puppet/string/option.rb
@@ -1,24 +1,69 @@
 class Puppet::String::Option
-  attr_reader :name, :string
+  attr_reader   :string
+  attr_reader   :name
+  attr_reader   :aliases
+  attr_accessor :desc
 
-  def initialize(string, name, attrs = {})
-    raise "#{name.inspect} is an invalid option name" unless name.to_s =~ /^[a-z]\w*$/
-    @string = string
-    @name   = name.to_sym
-    attrs.each do |k,v| send("#{k}=", v) end
+  def takes_argument?
+    !!@argument
+  end
+  def optional_argument?
+    !!@optional_argument
   end
 
+  def initialize(string, *declaration, &block)
+    @string   = string
+    @optparse = []
+
+    # Collect and sort the arguments in the declaration.
+    declaration.each do |item|
+      if item.is_a? String and item.to_s =~ /^-/ then
+        unless item =~ /^-[a-z]\b/ or item =~ /^--[^-]/ then
+          raise ArgumentError, "#{item.inspect}: long options need two dashes (--)"
+        end
+        @optparse << item
+      else
+        raise ArgumentError, "#{item.inspect} is not valid for an option argument"
+      end
+    end
+
+    if @optparse.empty? then
+      raise ArgumentError, "No option declarations found while building"
+    end
+
+    # Now, infer the name from the options; we prefer the first long option as
+    # the name, rather than just the first option.
+    @name = optparse_to_name(@optparse.find do |a| a =~ /^--/ end || @optparse.first)
+    @aliases = @optparse.map { |o| optparse_to_name(o) }
+
+    # Do we take an argument?  If so, are we consistent about it, because
+    # incoherence here makes our life super-difficult, and we can more easily
+    # relax this rule later if we find a valid use case for it. --daniel 2011-03-30
+    @argument = @optparse.any? { |o| o =~ /[ =]/ }
+    if @argument and not @optparse.all? { |o| o =~ /[ =]/ } then
+      raise ArgumentError, "Option #{@name} is inconsistent about taking an argument"
+    end
+
+    # Is our argument optional?  The rules about consistency apply here, also,
+    # just like they do to taking arguments at all. --daniel 2011-03-30
+    @optional_argument = @optparse.any? { |o| o.include? "[" }
+    if @optional_argument and not @optparse.all? { |o| o.include? "[" } then
+      raise ArgumentError, "Option #{@name} is inconsistent about the argument being optional"
+    end
+  end
+
+  # to_s and optparse_to_name are roughly mirrored, because they are used to
+  # transform strings to name symbols, and vice-versa.
   def to_s
     @name.to_s.tr('_', '-')
   end
 
-  Types = [:boolean, :string]
-  def type
-    @type ||= :boolean
-  end
-  def type=(input)
-    value = begin input.to_sym rescue nil end
-    Types.include?(value) or raise ArgumentError, "#{input.inspect} is not a valid type"
-    @type = value
+  def optparse_to_name(declaration)
+    unless found = declaration.match(/^-+([^= ]+)/) or found.length != 1 then
+      raise ArgumentError, "Can't find a name in the declaration #{declaration.inspect}"
+    end
+    name = found.captures.first.tr('-', '_')
+    raise "#{name.inspect} is an invalid option name" unless name.to_s =~ /^[a-z]\w*$/
+    name.to_sym
   end
 end
diff --git a/lib/puppet/string/option_builder.rb b/lib/puppet/string/option_builder.rb
index 2087cbc99..da0d213fb 100644
--- a/lib/puppet/string/option_builder.rb
+++ b/lib/puppet/string/option_builder.rb
@@ -3,14 +3,14 @@ require 'puppet/string/option'
 class Puppet::String::OptionBuilder
   attr_reader :option
 
-  def self.build(string, name, attrs = {}, &block)
-    new(string, name, attrs, &block).option
+  def self.build(string, *declaration, &block)
+    new(string, *declaration, &block).option
   end
 
   private
-  def initialize(string, name, attrs, &block)
+  def initialize(string, *declaration, &block)
     @string = string
-    @option = Puppet::String::Option.new(string, name, attrs)
+    @option = Puppet::String::Option.new(string, *declaration)
     block and instance_eval(&block)
     @option
   end
diff --git a/lib/puppet/string/option_manager.rb b/lib/puppet/string/option_manager.rb
index df3ae6b4b..f952ad4f0 100644
--- a/lib/puppet/string/option_manager.rb
+++ b/lib/puppet/string/option_manager.rb
@@ -3,16 +3,26 @@ require 'puppet/string/option_builder'
 module Puppet::String::OptionManager
   # Declare that this app can take a specific option, and provide
   # the code to do so.
-  def option(name, attrs = {}, &block)
-    @options ||= {}
-    raise ArgumentError, "Option #{name} already defined for #{self}" if option?(name)
-    actions.each do |action|
-      if get_action(action).option?(name) then
-        raise ArgumentError, "Option #{name} already defined on action #{action} for #{self}"
+  def option(*declaration, &block)
+    add_option Puppet::String::OptionBuilder.build(self, *declaration, &block)
+  end
+
+  def add_option(option)
+    option.aliases.each do |name|
+      if conflict = get_option(name) then
+        raise ArgumentError, "Option #{option} conflicts with existing option #{conflict}"
+      end
+
+      actions.each do |action|
+        action = get_action(action)
+        if conflict = action.get_option(name) then
+          raise ArgumentError, "Option #{option} conflicts with existing option #{conflict} on #{action}"
+        end
       end
     end
-    option = Puppet::String::OptionBuilder.build(self, name, &block)
-    @options[option.name] = option
+
+    option.aliases.each { |name| @options[name] = option }
+    option
   end
 
   def options