Merge branch 'feature/master/6749-actions-need-to-support-options'

33 files changed, 1013 insertions, 180 deletions

diff --git a/lib/puppet/application/configurer.rb b/lib/puppet/application/configurer.rb
index b440098ee..be018338f 100644
--- a/lib/puppet/application/configurer.rb
+++ b/lib/puppet/application/configurer.rb
@@ -5,8 +5,8 @@ class Puppet::Application::Configurer < Puppet::Application
   should_parse_config
   run_mode :agent
 
-  option("--debug","-d")
-  option("--verbose","-v")
+  option("--debug", "-d")
+  option("--verbose", "-v")
 
   def setup
     if options[:debug] or options[:verbose]
diff --git a/lib/puppet/application/indirection_base.rb b/lib/puppet/application/indirection_base.rb
index da61f408d..cfa1ea529 100644
--- a/lib/puppet/application/indirection_base.rb
+++ b/lib/puppet/application/indirection_base.rb
@@ -1,19 +1,4 @@
 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
-
-    if string.respond_to?(:indirection)
-      raise "Could not find data type #{type} for application #{self.class.name}" unless string.indirection
-
-      string.set_terminus(terminus) if terminus
-    end
-  end
 end
diff --git a/lib/puppet/application/string.rb b/lib/puppet/application/string.rb
index aa369e669..0a6a798ce 100644
--- a/lib/puppet/application/string.rb
+++ b/lib/puppet/application/string.rb
@@ -83,7 +83,7 @@ class Puppet::Application::String < Puppet::Application
   def actions(indirection)
     return [] unless string = Puppet::String[indirection, '0.0.1']
     string.load_actions
-    return string.actions.sort { |a,b| a.to_s <=> b.to_s }
+    return string.actions.sort { |a, b| a.to_s <=> b.to_s }
   end
 
   def load_applications
diff --git a/lib/puppet/application/string_base.rb b/lib/puppet/application/string_base.rb
index bc627adde..06e5789be 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
@@ -60,38 +44,85 @@ class Puppet::Application::StringBase < Puppet::Application
     end
   end
 
-  def setup
-    Puppet::Util::Log.newdestination :console
+  def preinit
+    super
+    trap(:INT) do
+      $stderr.puts "Cancelling String"
+      exit(0)
+    end
 
-    @verb = command_line.args.shift
-    @arguments = command_line.args
-    @arguments ||= []
+    # 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.
 
-    @arguments = Array(@arguments)
+    # 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, 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
+    index  = -1
+    until @action or (index += 1) >= command_line.args.length do
+      item = command_line.args[index]
+      if item =~ /^-/ then
+        option = @string.options.find { |a| item =~ /^-+#{a}\b/ }
+        if option then
+          option = @string.get_option(option)
+          # If we have an inline argument, just carry on.  We don't need to
+          # care about optional vs mandatory in that case because we do a real
+          # parse later, and that will totally take care of raising the error
+          # when we get there. --daniel 2011-04-04
+          if option.takes_argument? and !item.index('=') then
+            index += 1 unless
+              (option.optional_argument? and command_line.args[index + 1] =~ /^-/)
+          end
+        else
+          raise ArgumentError, "Unknown option #{item.sub(/=.*$/, '').inspect}"
+        end
+      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
 
-    @type = self.class.name.to_s.sub(/.+:/, '').downcase.to_sym
+    @action or raise ArgumentError, "No action given on the command line!"
 
-    # TODO: These should be configurable versions.
-    unless Puppet::String.string?(@type, :current)
-      raise "Could not find any version of string '#{@type}'"
+    # 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
-    @string = Puppet::String[@type, :current]
-    @format ||= @string.default_format
+  end
 
-    # We copy all of the app options to the string.
-    # This allows each action to read in the options.
-    @string.options = options
+  def setup
+    Puppet::Util::Log.newdestination :console
 
-    validate
+    # We copy all of the app options to the end of the call; This allows each
+    # 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
+    #
+    # Note: because of our definition of where the action is set, we end up
+    # with it *always* being the first word of the remaining set of command
+    # line arguments.  So, strip that off when we construct the arguments to
+    # pass down to the string action. --daniel 2011-04-04
+    @arguments = command_line.args[1, -1] || []
+    @arguments << options
   end
 
-  def validate
-    unless verb
-      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}"
+  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
 end
diff --git a/lib/puppet/string.rb b/lib/puppet/string.rb
index 783b6afe0..517cf4506 100644
--- a/lib/puppet/string.rb
+++ b/lib/puppet/string.rb
@@ -2,12 +2,16 @@ require 'puppet'
 require 'puppet/util/autoload'
 
 class Puppet::String
-  require 'puppet/string/action_manager'
   require 'puppet/string/string_collection'
 
+  require 'puppet/string/action_manager'
   include Puppet::String::ActionManager
   extend Puppet::String::ActionManager
 
+  require 'puppet/string/option_manager'
+  include Puppet::String::OptionManager
+  extend Puppet::String::OptionManager
+
   include Puppet::Util
 
   class << self
@@ -53,12 +57,12 @@ class Puppet::String
     self.default_format = format.to_sym
   end
 
-  attr_accessor :type, :verb, :version, :arguments, :options
+  attr_accessor :type, :verb, :version, :arguments
   attr_reader :name
 
   def initialize(name, version, &block)
     unless Puppet::String::StringCollection.validate_version(version)
-      raise ArgumentError, "Cannot create string with invalid version number '#{version}'!"
+      raise ArgumentError, "Cannot create string #{name.inspect} with invalid version number '#{version}'!"
     end
 
     @name = Puppet::String::StringCollection.underscorize(name)
diff --git a/lib/puppet/string/action.rb b/lib/puppet/string/action.rb
index 4db9e97e2..ee3b2991b 100644
--- a/lib/puppet/string/action.rb
+++ b/lib/puppet/string/action.rb
@@ -1,26 +1,121 @@
+# -*- coding: utf-8 -*-
 require 'puppet/string'
+require 'puppet/string/option'
 
 class Puppet::String::Action
   attr_reader :name
 
-  def initialize(string, name, attrs = {})
-    name = name.to_s
-    raise "'#{name}' is an invalid action name" unless name =~ /^[a-z]\w*$/
-
-    @string = string
-    @name      = name
-    attrs.each do |k,v| send("#{k}=", v) end
+  def to_s
+    "#{@string}##{@name}"
   end
 
-  def invoke(*args, &block)
-    @string.method(name).call(*args,&block)
+  def initialize(string, name, attrs = {})
+    raise "#{name.inspect} is an invalid action name" unless name.to_s =~ /^[a-z]\w*$/
+    @string  = string
+    @name    = name.to_sym
+    @options = {}
+    attrs.each do |k, v| send("#{k}=", v) end
   end
 
+  # Initially, this was defined to allow the @action.invoke pattern, which is
+  # a very natural way to invoke behaviour given our introspection
+  # capabilities.   Heck, our initial plan was to have the string delegate to
+  # the action object for invocation and all.
+  #
+  # It turns out that we have a binding problem to solve: @string was bound to
+  # the parent class, not the subclass instance, and we don't pass the
+  # appropriate context or change the binding enough to make this work.
+  #
+  # We could hack around it, by either mandating that you pass the context in
+  # to invoke, or try to get the binding right, but that has probably got
+  # subtleties that we don't instantly think of – especially around threads.
+  #
+  # So, we are pulling this method for now, and will return it to life when we
+  # have the time to resolve the problem.  For now, you should replace...
+  #
+  #     @action = @string.get_action(name)
+  #     @action.invoke(arg1, arg2, arg3)
+  #
+  # ...with...
+  #
+  #     @action = @string.get_action(name)
+  #     @string.send(@action.name, arg1, arg2, arg3)
+  #
+  # I understand that is somewhat cumbersome, but it functions as desired.
+  # --daniel 2011-03-31
+  #
+  # PS: This code is left present, but commented, to support this chunk of
+  # documentation, for the benefit of the reader.
+  #
+  # def invoke(*args, &block)
+  #   @string.send(name, *args, &block)
+  # end
+
   def invoke=(block)
+    # We need to build an instance method as a wrapper, using normal code, to
+    # be able to expose argument defaulting between the caller and definer in
+    # the Ruby API.  An extra method is, sadly, required for Ruby 1.8 to work.
+    #
+    # In future this also gives us a place to hook in additional behaviour
+    # such as calling out to the action instance to validate and coerce
+    # parameters, which avoids any exciting context switching and all.
+    #
+    # Hopefully we can improve this when we finally shuffle off the last of
+    # Ruby 1.8 support, but that looks to be a few "enterprise" release eras
+    # away, so we are pretty stuck with this for now.
+    #
+    # Patches to make this work more nicely with Ruby 1.9 using runtime
+    # version checking and all are welcome, but they can't actually help if
+    # the results are not totally hidden away in here.
+    #
+    # Incidentally, we though about vendoring evil-ruby and actually adjusting
+    # the internal C structure implementation details under the hood to make
+    # this stuff work, because it would have been cleaner.  Which gives you an
+    # idea how motivated we were to make this cleaner.  Sorry. --daniel 2011-03-31
+
+    internal_name = "#{@name} implementation, required on Ruby 1.8".to_sym
+    file = __FILE__ + "+eval"
+    line = __LINE__ + 1
+    wrapper = "def #{@name}(*args, &block)
+                 args << {} unless args.last.is_a? Hash
+                 args << block if block_given?
+                 self.__send__(#{internal_name.inspect}, *args)
+               end"
+
     if @string.is_a?(Class)
-      @string.define_method(@name, &block)
+      @string.class_eval do eval wrapper, nil, file, line end
+      @string.define_method(internal_name, &block)
     else
-      @string.meta_def(@name, &block)
+      @string.instance_eval do eval wrapper, nil, file, line end
+      @string.meta_def(internal_name, &block)
+    end
+  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}"
+      elsif conflict = @string.get_option(name) then
+        raise ArgumentError, "Option #{option} conflicts with existing option #{conflict} on #{@string}"
+      end
     end
+
+    option.aliases.each do |name|
+      @options[name] = option
+    end
+
+    option
+  end
+
+  def option?(name)
+    @options.include? name.to_sym
+  end
+
+  def options
+    (@options.keys + @string.options).sort
+  end
+
+  def get_option(name)
+    @options[name.to_sym] || @string.get_option(name)
   end
 end
diff --git a/lib/puppet/string/action_builder.rb b/lib/puppet/string/action_builder.rb
index b3db51104..e76044470 100644
--- a/lib/puppet/string/action_builder.rb
+++ b/lib/puppet/string/action_builder.rb
@@ -5,10 +5,8 @@ class Puppet::String::ActionBuilder
   attr_reader :action
 
   def self.build(string, name, &block)
-    name = name.to_s
-    raise "Action '#{name}' must specify a block" unless block
-    builder = new(string, name, &block)
-    builder.action
+    raise "Action #{name.inspect} must specify a block" unless block
+    new(string, name, &block).action
   end
 
   def initialize(string, name, &block)
@@ -24,4 +22,9 @@ class Puppet::String::ActionBuilder
     raise "Invoke called on an ActionBuilder with no corresponding Action" unless @action
     @action.invoke = block
   end
+
+  def option(*declaration, &block)
+    option = Puppet::String::OptionBuilder.build(@action, *declaration, &block)
+    @action.add_option(option)
+  end
 end
diff --git a/lib/puppet/string/action_manager.rb b/lib/puppet/string/action_manager.rb
index c29dbf454..7d22a0c52 100644
--- a/lib/puppet/string/action_manager.rb
+++ b/lib/puppet/string/action_manager.rb
@@ -5,20 +5,15 @@ module Puppet::String::ActionManager
   # the code to do so.
   def action(name, &block)
     @actions ||= {}
-    name = name.to_s.downcase.to_sym
-
     raise "Action #{name} already defined for #{self}" if action?(name)
-
     action = Puppet::String::ActionBuilder.build(self, name, &block)
-
-    @actions[name] = action
+    @actions[action.name] = action
   end
 
   # This is the short-form of an action definition; it doesn't use the
   # builder, just creates the action directly from the block.
   def script(name, &block)
     @actions ||= {}
-    name = name.to_s.downcase.to_sym
     raise "Action #{name} already defined for #{self}" if action?(name)
     @actions[name] = Puppet::String::Action.new(self, name, :invoke => block)
   end
@@ -36,7 +31,16 @@ module Puppet::String::ActionManager
   end
 
   def get_action(name)
-    @actions[name].dup
+    @actions ||= {}
+    result = @actions[name.to_sym]
+    if result.nil?
+      if self.is_a?(Class) and superclass.respond_to?(:get_action)
+        result = superclass.get_action(name)
+      elsif self.class.respond_to?(:get_action)
+        result = self.class.get_action(name)
+      end
+    end
+    return result
   end
 
   def action?(name)
diff --git a/lib/puppet/string/catalog.rb b/lib/puppet/string/catalog.rb
index 0ddd83176..c6de47708 100644
--- a/lib/puppet/string/catalog.rb
+++ b/lib/puppet/string/catalog.rb
@@ -2,7 +2,7 @@ require 'puppet/string/indirector'
 
 Puppet::String::Indirector.define(:catalog, '0.0.1') do
   action(:apply) do
-    invoke do |catalog|
+    invoke do |catalog, options|
       report = Puppet::Transaction::Report.new("apply")
       report.configuration_version = catalog.version
 
@@ -23,7 +23,7 @@ Puppet::String::Indirector.define(:catalog, '0.0.1') do
   end
 
   action(:download) do
-    invoke do |certname,facts|
+    invoke do |certname, facts, options|
       Puppet::Resource::Catalog.terminus_class = :rest
       facts_to_upload = {:facts_format => :b64_zlib_yaml, :facts => CGI.escape(facts.render(:b64_zlib_yaml))}
       catalog = nil
diff --git a/lib/puppet/string/catalog/select.rb b/lib/puppet/string/catalog/select.rb
index 52c77d3ce..a8f4480cd 100644
--- a/lib/puppet/string/catalog/select.rb
+++ b/lib/puppet/string/catalog/select.rb
@@ -1,7 +1,7 @@
 # Select and show a list of resources of a given type.
 Puppet::String.define(:catalog, '0.0.1') do
   action :select do
-    invoke do |host,type|
+    invoke do |host, type, options|
       catalog = Puppet::Resource::Catalog.indirection.find(host)
 
       catalog.resources.reject { |res| res.type != type }.each { |res| puts res }
diff --git a/lib/puppet/string/certificate.rb b/lib/puppet/string/certificate.rb
index 7b2e5f397..53f731e81 100644
--- a/lib/puppet/string/certificate.rb
+++ b/lib/puppet/string/certificate.rb
@@ -4,7 +4,7 @@ require 'puppet/ssl/host'
 Puppet::String::Indirector.define(:certificate, '0.0.1') do
 
   action :generate do
-    invoke do |name|
+    invoke do |name, options|
       host = Puppet::SSL::Host.new(name)
       host.generate_certificate_request
       host.certificate_request.class.indirection.save(host.certificate_request)
@@ -12,7 +12,7 @@ Puppet::String::Indirector.define(:certificate, '0.0.1') do
   end
 
   action :list do
-    invoke do
+    invoke do |options|
       Puppet::SSL::Host.indirection.search("*", {
         :for => :certificate_request,
       }).map { |h| h.inspect }
@@ -20,7 +20,7 @@ Puppet::String::Indirector.define(:certificate, '0.0.1') do
   end
 
   action :sign do
-    invoke do |name|
+    invoke do |name, options|
       Puppet::SSL::Host.indirection.save(Puppet::SSL::Host.new(name))
     end
   end
diff --git a/lib/puppet/string/config.rb b/lib/puppet/string/config.rb
index ae1a408cf..49a1688fc 100644
--- a/lib/puppet/string/config.rb
+++ b/lib/puppet/string/config.rb
@@ -3,6 +3,7 @@ require 'puppet/string'
 Puppet::String.define(:config, '0.0.1') do
   action(:print) do
     invoke do |*args|
+      options = args.pop
       Puppet.settings[:configprint] = args.join(",")
       Puppet.settings.print_config_options
       nil
diff --git a/lib/puppet/string/configurer.rb b/lib/puppet/string/configurer.rb
index a6ea74b6a..2520d4188 100644
--- a/lib/puppet/string/configurer.rb
+++ b/lib/puppet/string/configurer.rb
@@ -2,7 +2,7 @@ require 'puppet/string'
 
 Puppet::String.define(:configurer, '0.0.1') do
   action(:synchronize) do
-    invoke do |certname|
+    invoke do |certname, options|
       facts = Puppet::String[:facts, '0.0.1'].find(certname)
       catalog = Puppet::String[:catalog, '0.0.1'].download(certname, facts)
       report = Puppet::String[:catalog, '0.0.1'].apply(catalog)
diff --git a/lib/puppet/string/facts.rb b/lib/puppet/string/facts.rb
index 73acb0df6..31298813b 100644
--- a/lib/puppet/string/facts.rb
+++ b/lib/puppet/string/facts.rb
@@ -6,7 +6,7 @@ Puppet::String::Indirector.define(:facts, '0.0.1') do
 
   # Upload our facts to the server
   action(:upload) do
-    invoke do |*args|
+    invoke do |options|
       Puppet::Node::Facts.indirection.terminus_class = :facter
       facts = Puppet::Node::Facts.indirection.find(Puppet[:certname])
       Puppet::Node::Facts.indirection.terminus_class = :rest
diff --git a/lib/puppet/string/indirector.rb b/lib/puppet/string/indirector.rb
index 15984e39e..bb081533f 100644
--- a/lib/puppet/string/indirector.rb
+++ b/lib/puppet/string/indirector.rb
@@ -2,6 +2,11 @@ require 'puppet'
 require 'puppet/string'
 
 class Puppet::String::Indirector < Puppet::String
+  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
@@ -10,6 +15,21 @@ class Puppet::String::Indirector < Puppet::String
     Puppet::Indirector::Terminus.terminus_classes(indirection.to_sym).collect { |t| t.to_s }.sort
   end
 
+  def call_indirection_method(method, *args)
+    options = args.pop
+    options.has_key?(:terminus) and set_terminus(options[:terminus])
+
+    begin
+      result = indirection.__send__(method, *args)
+    rescue => detail
+      puts detail.backtrace if Puppet[:trace]
+      raise "Could not call '#{method}' on '#{indirection_name}': #{detail}"
+    end
+
+    indirection.reset_terminus_class
+    return result
+  end
+
   action :destroy do
     invoke { |*args| call_indirection_method(:destroy, *args) }
   end
@@ -29,11 +49,16 @@ class Puppet::String::Indirector < Puppet::String
   # Print the configuration for the current terminus class
   action :info do
     invoke do |*args|
+      options = args.pop
+      options.has_key?(:terminus) and set_terminus(options[:terminus])
+
       if t = indirection.terminus_class
         puts "Run mode '#{Puppet.run_mode.name}': #{t}"
       else
         $stderr.puts "No default terminus class for run mode '#{Puppet.run_mode.name}'"
       end
+
+      indirection.reset_terminus_class
     end
   end
 
@@ -53,7 +78,8 @@ class Puppet::String::Indirector < Puppet::String
   # One usually does.
   def indirection
     unless @indirection
-      Puppet.info("Could not find terminus for #{indirection_name}") unless @indirection = Puppet::Indirector::Indirection.instance(indirection_name)
+      @indirection = Puppet::Indirector::Indirection.instance(indirection_name)
+      @indirection or raise "Could not find terminus for #{indirection_name}"
     end
     @indirection
   end
@@ -62,18 +88,7 @@ class Puppet::String::Indirector < Puppet::String
     begin
       indirection.terminus_class = from
     rescue => detail
-      raise "Could not set '#{indirection.name}' terminus to '#{from}' (#{detail}); valid terminus types are #{terminus_classes(indirection.name).join(", ") }"
+      raise "Could not set '#{indirection.name}' terminus to '#{from}' (#{detail}); valid terminus types are #{self.class.terminus_classes(indirection.name).join(", ") }"
     end
   end
-
-  def call_indirection_method(method, *args)
-    begin
-      result = indirection.send(method, *args)
-    rescue => detail
-      puts detail.backtrace if Puppet[:trace]
-      raise "Could not call '#{method}' on '#{indirection_name}': #{detail}"
-    end
-
-    result
-  end
 end
diff --git a/lib/puppet/string/option.rb b/lib/puppet/string/option.rb
new file mode 100644
index 000000000..f499e4b95
--- /dev/null
+++ b/lib/puppet/string/option.rb
@@ -0,0 +1,80 @@
+class Puppet::String::Option
+  attr_reader   :parent
+  attr_reader   :name
+  attr_reader   :aliases
+  attr_reader   :optparse
+  attr_accessor :desc
+
+  def takes_argument?
+    !!@argument
+  end
+  def optional_argument?
+    !!@optional_argument
+  end
+
+  def initialize(parent, *declaration, &block)
+    @parent   = parent
+    @optparse = []
+
+    # Collect and sort the arguments in the declaration.
+    dups = {}
+    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
+
+        # Duplicate checking...
+        name = optparse_to_name(item)
+        if dup = dups[name] then
+          raise ArgumentError, "#{item.inspect}: duplicates existing alias #{dup.inspect} in #{@parent}"
+        else
+          dups[name] = item
+        end
+      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.  This isn't a full
+  # bidirectional transformation though.
+  def to_s
+    @name.to_s.tr('_', '-')
+  end
+
+  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.sub('[no-]', '').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
new file mode 100644
index 000000000..da0d213fb
--- /dev/null
+++ b/lib/puppet/string/option_builder.rb
@@ -0,0 +1,25 @@
+require 'puppet/string/option'
+
+class Puppet::String::OptionBuilder
+  attr_reader :option
+
+  def self.build(string, *declaration, &block)
+    new(string, *declaration, &block).option
+  end
+
+  private
+  def initialize(string, *declaration, &block)
+    @string = string
+    @option = Puppet::String::Option.new(string, *declaration)
+    block and instance_eval(&block)
+    @option
+  end
+
+  # Metaprogram the simple DSL from the option class.
+  Puppet::String::Option.instance_methods.grep(/=$/).each do |setter|
+    next if setter =~ /^=/      # special case, darn it...
+
+    dsl = setter.sub(/=$/, '')
+    define_method(dsl) do |value| @option.send(setter, value) end
+  end
+end
diff --git a/lib/puppet/string/option_manager.rb b/lib/puppet/string/option_manager.rb
new file mode 100644
index 000000000..f952ad4f0
--- /dev/null
+++ b/lib/puppet/string/option_manager.rb
@@ -0,0 +1,56 @@
+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(*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.aliases.each { |name| @options[name] = option }
+    option
+  end
+
+  def options
+    @options ||= {}
+    result = @options.keys
+
+    if self.is_a?(Class) and superclass.respond_to?(:options)
+      result += superclass.options
+    elsif self.class.respond_to?(:options)
+      result += self.class.options
+    end
+    result.sort
+  end
+
+  def get_option(name)
+    @options ||= {}
+    result = @options[name.to_sym]
+    unless result then
+      if self.is_a?(Class) and superclass.respond_to?(:get_option)
+        result = superclass.get_option(name)
+      elsif self.class.respond_to?(:get_option)
+        result = self.class.get_option(name)
+      end
+    end
+    return result
+  end
+
+  def option?(name)
+    options.include? name.to_sym
+  end
+end
diff --git a/lib/puppet/string/report.rb b/lib/puppet/string/report.rb
index 55a008533..5b617e49e 100644
--- a/lib/puppet/string/report.rb
+++ b/lib/puppet/string/report.rb
@@ -2,7 +2,7 @@ require 'puppet/string/indirector'
 
 Puppet::String::Indirector.define(:report, '0.0.1') do
   action(:submit) do
-    invoke do |report|
+    invoke do |report, options|
       begin
         Puppet::Transaction::Report.terminus_class = :rest
         report.save
diff --git a/spec/shared_behaviours/things_that_declare_options.rb b/spec/shared_behaviours/things_that_declare_options.rb
new file mode 100644
index 000000000..1b41c2279
--- /dev/null
+++ b/spec/shared_behaviours/things_that_declare_options.rb
@@ -0,0 +1,134 @@
+# -*- coding: utf-8 -*-
+shared_examples_for "things that declare options" do
+  it "should support options without arguments" do
+    subject = add_options_to { option "--bar" }
+    subject.should be_option :bar
+  end
+
+  it "should support options with an empty block" do
+    subject = add_options_to do
+      option "--foo" do
+        # this section deliberately left blank
+      end
+    end
+    subject.should be
+    subject.should be_option :foo
+  end
+
+  it "should support option documentation" do
+    text = "Sturm und Drang (German pronunciation: [ˈʃtʊʁm ʊnt ˈdʁaŋ]) …"
+
+    subject = add_options_to do
+      option "--foo" do
+        desc text
+      end
+    end
+
+    subject.get_option(:foo).desc.should == text
+  end
+
+  it "should list all the options" do
+    subject = add_options_to do
+      option "--foo"
+      option "--bar"
+    end
+    subject.options.should =~ [:foo, :bar]
+  end
+
+  it "should detect conflicts in long options" do
+    expect {
+      add_options_to do
+        option "--foo"
+        option "--foo"
+      end
+    }.should raise_error ArgumentError, /Option foo conflicts with existing option foo/i
+  end
+
+  it "should detect conflicts in short options" do
+    expect {
+      add_options_to do
+        option "-f"
+        option "-f"
+      end
+    }.should raise_error ArgumentError, /Option f conflicts with existing option f/
+  end
+
+  ["-f", "--foo"].each do |option|
+    ["", " FOO", "=FOO", " [FOO]", "=[FOO]"].each do |argument|
+      input = option + argument
+      it "should detect conflicts within a single option like #{input.inspect}" do
+        expect {
+          add_options_to do
+            option input, input
+          end
+        }.should raise_error ArgumentError, /duplicates existing alias/
+      end
+    end
+  end
+
+
+  # Verify the range of interesting conflicts to check for ordering causing
+  # the behaviour to change, or anything exciting like that.
+  [ %w{--foo}, %w{-f}, %w{-f --foo}, %w{--baz -f},
+    %w{-f --baz}, %w{-b --foo}, %w{--foo -b}
+  ].each do |conflict|
+    base = %w{--foo -f}
+    it "should detect conflicts between #{base.inspect} and #{conflict.inspect}" do
+      expect {
+        add_options_to do
+          option *base
+          option *conflict
+        end
+      }.should raise_error ArgumentError, /conflicts with existing option/
+    end
+  end
+
+  it "should fail if we are not consistent about taking an argument" do
+    expect { add_options_to do option "--foo=bar", "--bar" end }.
+      should raise_error ArgumentError, /inconsistent about taking an argument/
+  end
+
+  it "should accept optional arguments" do
+    subject = add_options_to do option "--foo=[baz]", "--bar=[baz]" end
+    [:foo, :bar].each do |name|
+      subject.should be_option name
+    end
+  end
+
+  describe "#takes_argument?" do
+    it "should detect an argument being absent" do
+      subject = add_options_to do option "--foo" end
+      subject.get_option(:foo).should_not be_takes_argument
+    end
+    ["=FOO", " FOO", "=[FOO]", " [FOO]"].each do |input|
+      it "should detect an argument given #{input.inspect}" do
+        subject = add_options_to do option "--foo#{input}" end
+        subject.get_option(:foo).should be_takes_argument
+      end
+    end
+  end
+
+  describe "#optional_argument?" do
+    it "should be false if no argument is present" do
+      option = add_options_to do option "--foo" end.get_option(:foo)
+      option.should_not be_takes_argument
+      option.should_not be_optional_argument
+    end
+
+    ["=FOO", " FOO"].each do |input|
+      it "should be false if the argument is mandatory (like #{input.inspect})" do
+        option = add_options_to do option "--foo#{input}" end.get_option(:foo)
+      option.should be_takes_argument
+      option.should_not be_optional_argument
+      end
+    end
+
+    ["=[FOO]", " [FOO]"].each do |input|
+      it "should be true if the argument is optional (like #{input.inspect})" do
+        option = add_options_to do option "--foo#{input}" end.get_option(:foo)
+      option.should be_takes_argument
+      option.should be_optional_argument
+      end
+    end
+  end
+end
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index 4e54d7235..4073cb60b 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -7,6 +7,10 @@ require 'puppet'
 require 'puppet/string'
 require 'rspec'
 
+Pathname.glob("#{dir}/shared_behaviours/**/*.rb") do |behaviour|
+  require behaviour.relative_path_from(dir)
+end
+
 RSpec.configure do |config|
     config.mock_with :mocha
 
diff --git a/spec/unit/application/certificate_spec.rb b/spec/unit/application/certificate_spec.rb
index 0688666de..6666f54f7 100644..100755
--- a/spec/unit/application/certificate_spec.rb
+++ b/spec/unit/application/certificate_spec.rb
@@ -13,9 +13,4 @@ describe Puppet::Application::Certificate do
   it "should have a 'ca' option" do
     Puppet::Application::Certificate.new.should respond_to(:handle_ca_location)
   end
-
-  it "should set the CA location using the 'ca' option" do
-    Puppet::Application::Certificate.new.handle_ca_location("local")
-    Puppet::SSL::Host.indirection.terminus_class.should == :file
-  end
 end
diff --git a/spec/unit/application/configurer_spec.rb b/spec/unit/application/configurer_spec.rb
index 621039bcc..621039bcc 100644..100755
--- a/spec/unit/application/configurer_spec.rb
+++ b/spec/unit/application/configurer_spec.rb
diff --git a/spec/unit/application/indirection_base_spec.rb b/spec/unit/application/indirection_base_spec.rb
index ecc49d9a9..66b3009fb 100755
--- a/spec/unit/application/indirection_base_spec.rb
+++ b/spec/unit/application/indirection_base_spec.rb
@@ -2,11 +2,38 @@
 
 require File.expand_path(File.dirname(__FILE__) + '/../../spec_helper.rb')
 require 'puppet/application/indirection_base'
+require 'puppet/string/indirector'
+
+########################################################################
+# Stub for testing; the names are critical, sadly. --daniel 2011-03-30
+class Puppet::Application::TestIndirection < Puppet::Application::IndirectionBase
+end
+
+string = Puppet::String::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
+string.version = :current
+Puppet::String.register(string)
+########################################################################
+
 
 describe Puppet::Application::IndirectionBase do
-  it "should support a 'from' terminus"
+  subject { Puppet::Application::TestIndirection.new }
+
+  it "should accept a terminus command line option" do
+    # It would be nice not to have to stub this, but whatever... writing an
+    # entire indirection stack would cause us more grief. --daniel 2011-03-31
+    terminus = mock("test indirection terminus")
+    Puppet::Indirector::Indirection.expects(:instance).
+      with(:testindirection).twice.returns()
+
+    subject.command_line.
+      instance_variable_set('@args', %w{--terminus foo save})
+
+    # Not a very nice thing. :(
+    $stderr.stubs(:puts)
 
-  describe "setup" do
-    it "should fail if its string does not support an indirection"
+    expect { subject.run }.should raise_error SystemExit
   end
 end
diff --git a/spec/unit/application/string_base_spec.rb b/spec/unit/application/string_base_spec.rb
index 86f9c09aa..71e67283d 100755
--- a/spec/unit/application/string_base_spec.rb
+++ b/spec/unit/application/string_base_spec.rb
@@ -15,6 +15,17 @@ describe Puppet::Application::StringBase do
     File.open(File.join(@dir, 'puppet', 'string', 'basetest.rb'), 'w') do |f|
       f.puts "Puppet::String.define(:basetest, '0.0.1')"
     end
+
+    Puppet::String.define(:basetest, '0.0.1') do
+      option("--[no-]boolean")
+      option("--mandatory MANDATORY")
+      option("--optional [OPTIONAL]")
+
+      action :foo do
+        option("--action")
+        invoke { |*args| args.length }
+      end
+    end
   end
 
   after :all do
@@ -22,53 +33,118 @@ 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'
     Puppet::Util::Log.stubs(:newdestination)
+    app
   end
 
-  describe "when calling main" do
-    before do
-      @app.verb = :find
-      @app.arguments = ["myname", "myarg"]
-      @app.string.stubs(:find)
+  describe "#preinit" do
+    before :each do
+      app.command_line.stubs(:args).returns %w{}
     end
 
-    it "should send the specified verb and name to the string" do
-      @app.string.expects(:find).with("myname", "myarg")
+    describe "parsing the command line" do
+      context "with just an action" do
+        before :all do
+          app.command_line.stubs(:args).returns %w{foo}
+          app.preinit
+        end
 
-      @app.main
-    end
+        it "should set the string based on the type" do
+          app.string.name.should == :basetest
+        end
 
-    it "should use its render method to render any result"
+        it "should set the format based on the string default" do
+          app.format.should == :pson
+        end
 
-    it "should exit with the current exit code"
-  end
+        it "should find the action" do
+          app.action.should be
+          app.action.name.should == :foo
+        end
+      end
 
-  describe "during setup" do
-    before do
-      @app.command_line.stubs(:args).returns(["find", "myname", "myarg"])
-      @app.stubs(:validate)
+      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{--action=bar foo}
+        expect { app.preinit }.
+          should raise_error ArgumentError, /Unknown 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"/
+      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
+
+      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.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.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"/
+      end
     end
+  end
 
-    it "should set the verb from the command line arguments" do
-      @app.setup
-      @app.verb.should == "find"
+  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.arguments.should == [{ :mandatory => "--bar" }]
     end
+  end
 
-    it "should make sure arguments are an array" do
-      @app.command_line.stubs(:args).returns(["find", "myname", "myarg"])
-      @app.setup
-      @app.arguments.should == ["myname", "myarg"]
+  describe "#main" do
+    before do
+      app.string    = Puppet::String[:basetest, '0.0.1']
+      app.action    = app.string.get_action(:foo)
+      app.format    = :pson
+      app.arguments = ["myname", "myarg"]
     end
 
-    it "should set the options on the string" do
-      @app.options[:foo] = "bar"
-      @app.setup
+    it "should send the specified verb and name to the string" do
+      app.string.expects(:foo).with(*app.arguments)
+      app.main
+    end
 
-      @app.string.options.should == @app.options
+    it "should use its render method to render any result" do
+      app.expects(:render).with(app.arguments.length + 1)
+      app.main
     end
   end
 end
diff --git a/spec/unit/string/action_builder_spec.rb b/spec/unit/string/action_builder_spec.rb
index c3395cf6a..fde010d51 100755
--- a/spec/unit/string/action_builder_spec.rb
+++ b/spec/unit/string/action_builder_spec.rb
@@ -6,10 +6,10 @@ require 'puppet/string/action_builder'
 describe Puppet::String::ActionBuilder do
   describe "::build" do
     it "should build an action" do
-      action = Puppet::String::ActionBuilder.build(nil,:foo) do
+      action = Puppet::String::ActionBuilder.build(nil, :foo) do
       end
       action.should be_a(Puppet::String::Action)
-      action.name.should == "foo"
+      action.name.should == :foo
     end
 
     it "should define a method on the string which invokes the action" do
@@ -24,7 +24,36 @@ describe Puppet::String::ActionBuilder do
     end
 
     it "should require a block" do
-      lambda { Puppet::String::ActionBuilder.build(nil,:foo) }.should raise_error("Action 'foo' must specify a block")
+      lambda { Puppet::String::ActionBuilder.build(nil, :foo) }.
+        should raise_error("Action :foo must specify a block")
+    end
+
+    describe "when handling options" do
+      let :string do Puppet::String.new(:option_handling, '0.0.1') end
+
+      it "should have a #option DSL function" do
+        method = nil
+        Puppet::String::ActionBuilder.build(string, :foo) do
+          method = self.method(:option)
+        end
+        method.should be
+      end
+
+      it "should define an option without a block" do
+        action = Puppet::String::ActionBuilder.build(string, :foo) do
+          option "--bar"
+        end
+        action.should be_option :bar
+      end
+
+      it "should accept an empty block" do
+        action = Puppet::String::ActionBuilder.build(string, :foo) do
+          option "--bar" do
+            # This space left deliberately blank.
+          end
+        end
+        action.should be_option :bar
+      end
     end
   end
 end
diff --git a/spec/unit/string/action_manager_spec.rb b/spec/unit/string/action_manager_spec.rb
index 3921f02c0..5ca55b387 100755
--- a/spec/unit/string/action_manager_spec.rb
+++ b/spec/unit/string/action_manager_spec.rb
@@ -213,4 +213,21 @@ describe Puppet::String::ActionManager do
       @instance.foo.should == "something"
     end
   end
+
+  describe "#get_action" do
+    let :parent_class do
+      parent_class = Class.new(Puppet::String)
+      parent_class.action(:foo) {}
+      parent_class
+    end
+
+    it "should check that we can find inherited actions when we are a class" do
+      Class.new(parent_class).get_action(:foo).name.should == :foo
+    end
+
+    it "should check that we can find inherited actions when we are an instance" do
+      instance = parent_class.new(:foo, '0.0.0')
+      instance.get_action(:foo).name.should == :foo
+    end
+  end
 end
diff --git a/spec/unit/string/action_spec.rb b/spec/unit/string/action_spec.rb
index 4026c9a58..e5fefdbdc 100755
--- a/spec/unit/string/action_spec.rb
+++ b/spec/unit/string/action_spec.rb
@@ -5,20 +5,11 @@ require 'puppet/string/action'
 
 describe Puppet::String::Action do
   describe "when validating the action name" do
-    it "should require a name" do
-      lambda { Puppet::String::Action.new(nil,nil) }.should raise_error("'' is an invalid action name")
-    end
-
-    it "should not allow empty names" do
-      lambda { Puppet::String::Action.new(nil,'') }.should raise_error("'' is an invalid action name")
-    end
-
-    it "should not allow names with whitespace" do
-      lambda { Puppet::String::Action.new(nil,'foo bar') }.should raise_error("'foo bar' is an invalid action name")
-    end
-
-    it "should not allow names beginning with dashes" do
-      lambda { Puppet::String::Action.new(nil,'-foobar') }.should raise_error("'-foobar' is an invalid action name")
+    [nil, '', 'foo bar', '-foobar'].each do |input|
+      it "should treat #{input.inspect} as an invalid name" do
+        expect { Puppet::String::Action.new(nil, input) }.
+          should raise_error(/is an invalid action name/)
+      end
     end
   end
 
@@ -71,5 +62,96 @@ 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
+    it "should support options with an empty block" do
+      string = Puppet::String.new(:action_level_options, '0.0.1') do
+        action :foo do
+          option "--bar" do
+            # this line left deliberately blank
+          end
+        end
+      end
+
+      string.should_not be_option :bar
+      string.get_action(:foo).should be_option :bar
+    end
+
+    it "should return only action level options when there are no string options" do
+      string = Puppet::String.new(:action_level_options, '0.0.1') do
+        action :foo do option "--bar" end
+      end
+
+      string.get_action(:foo).options.should =~ [:bar]
+    end
+
+    describe "with both string and action options" do
+      let :string do
+        Puppet::String.new(:action_level_options, '0.0.1') do
+          action :foo do option "--bar" end
+          action :baz do option "--bim" end
+          option "--quux"
+        end
+      end
+
+      it "should return combined string and action options" do
+        string.get_action(:foo).options.should =~ [:bar, :quux]
+      end
+
+      it "should get an action option when asked" do
+        string.get_action(:foo).get_option(:bar).
+          should be_an_instance_of Puppet::String::Option
+      end
+
+      it "should get a string option when asked" do
+        string.get_action(:foo).get_option(:quux).
+          should be_an_instance_of Puppet::String::Option
+      end
+
+      it "should return options only for this action" do
+        string.get_action(:baz).options.should =~ [:bim, :quux]
+      end
+    end
+
+    it_should_behave_like "things that declare options" do
+      def add_options_to(&block)
+        string = Puppet::String.new(:with_options, '0.0.1') do
+          action(:foo, &block)
+        end
+        string.get_action(:foo)
+      end
+    end
+
+    it "should fail when a string option duplicates an action option" do
+      expect {
+        Puppet::String.new(:action_level_options, '0.0.1') do
+          option "--foo"
+          action :bar do option "--foo" end
+        end
+      }.should raise_error ArgumentError, /Option foo conflicts with existing option foo/i
+    end
   end
 end
diff --git a/spec/unit/string/indirector_spec.rb b/spec/unit/string/indirector_spec.rb
index 89306c416..cb85eaa05 100755
--- a/spec/unit/string/indirector_spec.rb
+++ b/spec/unit/string/indirector_spec.rb
@@ -4,12 +4,13 @@ require File.expand_path(File.dirname(__FILE__) + '/../../spec_helper.rb')
 require 'puppet/string/indirector'
 
 describe Puppet::String::Indirector do
-  before do
-    @instance = Puppet::String::Indirector.new(:test, '0.0.1')
-
-    @indirection = stub 'indirection', :name => :stub_indirection
-
-    @instance.stubs(:indirection).returns @indirection
+  subject do
+    instance = Puppet::String::Indirector.new(:test, '0.0.1')
+    indirection = stub('indirection',
+                       :name => :stub_indirection,
+                       :reset_terminus_class => nil)
+    instance.stubs(:indirection).returns indirection
+    instance
   end
 
   it "should be able to return a list of indirections" do
@@ -33,20 +34,20 @@ describe Puppet::String::Indirector do
       Puppet::String::Indirector.should be_action(method)
     end
 
-    it "should just call the indirection method when the '#{method}' action is invoked" do
-      @instance.indirection.expects(method).with(:test, "myargs")
-      @instance.send(method, :test, "myargs")
+    it "should call the indirection method when the '#{method}' action is invoked" do
+      subject.indirection.expects(method).with(:test, "myargs")
+      subject.send(method, :test, "myargs")
     end
   end
 
   it "should be able to override its indirection name" do
-    @instance.set_indirection_name :foo
-    @instance.indirection_name.should == :foo
+    subject.set_indirection_name :foo
+    subject.indirection_name.should == :foo
   end
 
   it "should be able to set its terminus class" do
-    @instance.indirection.expects(:terminus_class=).with(:myterm)
-    @instance.set_terminus(:myterm)
+    subject.indirection.expects(:terminus_class=).with(:myterm)
+    subject.set_terminus(:myterm)
   end
 
   it "should define a class-level 'info' action" do
diff --git a/spec/unit/string/option_builder_spec.rb b/spec/unit/string/option_builder_spec.rb
new file mode 100644
index 000000000..9e913c29c
--- /dev/null
+++ b/spec/unit/string/option_builder_spec.rb
@@ -0,0 +1,29 @@
+require 'puppet/string/option_builder'
+
+describe Puppet::String::OptionBuilder do
+  let :string do Puppet::String.new(:option_builder_testing, '0.0.1') end
+
+  it "should be able to construct an option without a block" do
+    Puppet::String::OptionBuilder.build(string, "--foo").
+      should be_an_instance_of Puppet::String::Option
+  end
+
+  describe "when using the DSL block" do
+    it "should work with an empty block" do
+      option = Puppet::String::OptionBuilder.build(string, "--foo") do
+        # This block deliberately left blank.
+      end
+
+      option.should be_an_instance_of Puppet::String::Option
+    end
+
+    it "should support documentation declarations" do
+      text = "this is the description"
+      option = Puppet::String::OptionBuilder.build(string, "--foo") do
+        desc text
+      end
+      option.should be_an_instance_of Puppet::String::Option
+      option.desc.should == text
+    end
+  end
+end
diff --git a/spec/unit/string/option_spec.rb b/spec/unit/string/option_spec.rb
new file mode 100644
index 000000000..f4f62ec37
--- /dev/null
+++ b/spec/unit/string/option_spec.rb
@@ -0,0 +1,75 @@
+require 'puppet/string/option'
+
+describe Puppet::String::Option do
+  let :string do Puppet::String.new(:option_testing, '0.0.1') end
+
+  describe "#optparse_to_name" do
+    ["", "=BAR", " BAR", "=bar", " bar"].each do |postfix|
+      { "--foo" => :foo, "-f" => :f }.each do |base, expect|
+        input = base + postfix
+        it "should map #{input.inspect} to #{expect.inspect}" do
+          option = Puppet::String::Option.new(string, input)
+          option.name.should == expect
+        end
+      end
+    end
+
+    [:foo, 12, nil, {}, []].each do |input|
+      it "should fail sensible when given #{input.inspect}" do
+        expect { Puppet::String::Option.new(string, input) }.
+          should raise_error ArgumentError, /is not valid for an option argument/
+      end
+    end
+
+    ["-foo", "-foo=BAR", "-foo BAR"].each do |input|
+      it "should fail with a single dash for long option #{input.inspect}" do
+        expect { Puppet::String::Option.new(string, input) }.
+          should raise_error ArgumentError, /long options need two dashes \(--\)/
+      end
+    end
+  end
+
+  it "requires a string when created" do
+    expect { Puppet::String::Option.new }.
+      should raise_error ArgumentError, /wrong number of arguments/
+  end
+
+  it "also requires some declaration arguments when created" do
+    expect { Puppet::String::Option.new(string) }.
+      should raise_error ArgumentError, /No option declarations found/
+  end
+
+  it "should infer the name from an optparse string" do
+    option = Puppet::String::Option.new(string, "--foo")
+    option.name.should == :foo
+  end
+
+  it "should infer the name when multiple optparse strings are given" do
+    option = Puppet::String::Option.new(string, "--foo", "-f")
+    option.name.should == :foo
+  end
+
+  it "should prefer the first long option name over a short option name" do
+    option = Puppet::String::Option.new(string, "-f", "--foo")
+    option.name.should == :foo
+  end
+
+  it "should create an instance when given a string and name" do
+    Puppet::String::Option.new(string, "--foo").
+      should be_instance_of Puppet::String::Option
+  end
+
+  describe "#to_s" do
+    it "should transform a symbol into a string" do
+      option = Puppet::String::Option.new(string, "--foo")
+      option.name.should == :foo
+      option.to_s.should == "foo"
+    end
+
+    it "should use - rather than _ to separate words in strings but not symbols" do
+      option = Puppet::String::Option.new(string, "--foo-bar")
+      option.name.should == :foo_bar
+      option.to_s.should == "foo-bar"
+    end
+  end
+end
diff --git a/spec/unit/string/string_collection_spec.rb b/spec/unit/string/string_collection_spec.rb
index 63ddf7c5e..184299e3c 100755
--- a/spec/unit/string/string_collection_spec.rb
+++ b/spec/unit/string/string_collection_spec.rb
@@ -101,7 +101,8 @@ describe Puppet::String::StringCollection do
     end
 
     it "should require the string by version if the 'current' version isn't it" do
-      subject.expects(:require).with('puppet/string/bar').raises(LoadError)
+      subject.expects(:require).with('puppet/string/bar').
+        raises(LoadError, 'no such file to load -- puppet/string/bar')
       subject.expects(:require).with do |file|
         @strings[:bar]['0.0.1'] = true
         file == 'bar@0.0.1/puppet/string/bar'
@@ -115,7 +116,9 @@ describe Puppet::String::StringCollection do
     end
 
     it "should return false if there is a LoadError requiring the string" do
-      subject.stubs(:require).raises(LoadError)
+      subject.stubs(:require).
+        raises(LoadError, 'no such file to load -- puppet/string/bar').then.
+        raises(LoadError, 'no such file to load -- bar@0.0.1/puppet/string/bar')
       subject.string?("bar", '0.0.1').should == false
     end
 
@@ -138,7 +141,8 @@ describe Puppet::String::StringCollection do
     end
 
     it "should not register the version loaded from `{name}@{version}` as `:current`" do
-      subject.expects(:require).with('puppet/string/huzzah').raises(LoadError)
+      subject.expects(:require).with('puppet/string/huzzah').
+        raises(LoadError, 'no such file to load -- puppet/string/huzzah')
       subject.expects(:require).with do |file|
         @strings[:huzzah]['0.0.1'] = true
         file == 'huzzah@0.0.1/puppet/string/huzzah'
diff --git a/spec/unit/string_spec.rb b/spec/unit/string_spec.rb
index 64d4f12f8..ddf855475 100755
--- a/spec/unit/string_spec.rb
+++ b/spec/unit/string_spec.rb
@@ -41,7 +41,7 @@ describe Puppet::String do
     end
 
     it "should instance-eval any provided block" do
-      face = Puppet::String.new(:string_test_block,'0.0.1') do
+      face = Puppet::String.new(:string_test_block, '0.0.1') do
         action(:something) do
           invoke { "foo" }
         end
@@ -52,15 +52,15 @@ describe Puppet::String do
   end
 
   it "should have a name" do
-    Puppet::String.new(:me,'0.0.1').name.should == :me
+    Puppet::String.new(:me, '0.0.1').name.should == :me
   end
 
   it "should stringify with its own name" do
-    Puppet::String.new(:me,'0.0.1').to_s.should =~ /\bme\b/
+    Puppet::String.new(:me, '0.0.1').to_s.should =~ /\bme\b/
   end
 
   it "should allow overriding of the default format" do
-    face = Puppet::String.new(:me,'0.0.1')
+    face = Puppet::String.new(:me, '0.0.1')
     face.set_default_format :foo
     face.default_format.should == :foo
   end
@@ -81,4 +81,65 @@ describe Puppet::String do
   end
 
   it "should be able to load all actions in all search paths"
+
+
+  it_should_behave_like "things that declare options" do
+    def add_options_to(&block)
+      Puppet::String.new(:with_options, '0.0.1', &block)
+    end
+  end
+
+  describe "with string-level options" do
+    it "should not return any action-level options" do
+      string = Puppet::String.new(:with_options, '0.0.1') do
+        option "--foo"
+        option "--bar"
+        action :baz do
+          option "--quux"
+        end
+      end
+      string.options.should =~ [:foo, :bar]
+    end
+
+    it "should fail when a string option duplicates an action option" do
+      expect {
+        Puppet::String.new(:action_level_options, '0.0.1') do
+          action :bar do option "--foo" end
+          option "--foo"
+        end
+      }.should raise_error ArgumentError, /Option foo conflicts with existing option foo on/i
+    end
+
+    it "should work when two actions have the same option" do
+      string = Puppet::String.new(:with_options, '0.0.1') do
+        action :foo do option "--quux" end
+        action :bar do option "--quux" end
+      end
+
+      string.get_action(:foo).options.should =~ [:quux]
+      string.get_action(:bar).options.should =~ [:quux]
+    end
+  end
+
+  describe "with inherited options" do
+    let :string do
+      parent = Class.new(Puppet::String)
+      parent.option("--inherited")
+      string = parent.new(:example, '0.2.1')
+      string.option("--local")
+      string
+    end
+
+    describe "#options" do
+      it "should list inherited options" do
+        string.options.should =~ [:inherited, :local]
+      end
+    end
+
+    describe "#get_option" do
+      it "should return an inherited option object" do
+        string.get_option(:inherited).should be_an_instance_of Puppet::String::Option
+      end
+    end
+  end
 end