(#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>

16 files changed, 377 insertions, 204 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
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..6abce99e3
--- /dev/null
+++ b/spec/shared_behaviours/things_that_declare_options.rb
@@ -0,0 +1,120 @@
+# -*- 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
+
+  # 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/indirection_base_spec.rb b/spec/unit/application/indirection_base_spec.rb
index 53e5f7e76..f636613c4 100755
--- a/spec/unit/application/indirection_base_spec.rb
+++ b/spec/unit/application/indirection_base_spec.rb
@@ -2,21 +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 'terminus' accessor" do
-    test = subject
-    expect { test.terminus = :foo }.should_not raise_error
-    test.terminus.should == :foo
-  end
+  subject { Puppet::Application::TestIndirection.new }
 
-  it "should have a 'terminus' CLI option" do
-    subject.class.option_parser_commands.select do |options, function|
-      options.index { |o| o =~ /terminus/ }
-    end.should_not be_empty
-  end
+  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).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/string/action_builder_spec.rb b/spec/unit/string/action_builder_spec.rb
index 946244cbf..0229fe44d 100755
--- a/spec/unit/string/action_builder_spec.rb
+++ b/spec/unit/string/action_builder_spec.rb
@@ -41,14 +41,14 @@ describe Puppet::String::ActionBuilder do
 
       it "should define an option without a block" do
         action = Puppet::String::ActionBuilder.build(string, :foo) do
-          option :bar
+          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
+          option "--bar" do
             # This space left deliberately blank.
           end
         end
diff --git a/spec/unit/string/action_spec.rb b/spec/unit/string/action_spec.rb
index d182f0abe..258ad5aa6 100755
--- a/spec/unit/string/action_spec.rb
+++ b/spec/unit/string/action_spec.rb
@@ -65,21 +65,10 @@ describe Puppet::String::Action do
   end
 
   describe "with action-level options" do
-    it "should support options without arguments" do
-      string = Puppet::String.new(:action_level_options, '0.0.1') do
-        action(:foo) do
-          option :bar
-        end
-      end
-
-      string.should_not be_option :bar
-      string.get_action(:foo).should be_option :bar
-    end
-
     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
+          option "--bar" do
             # this line left deliberately blank
           end
         end
@@ -91,7 +80,7 @@ describe Puppet::String::Action do
 
     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
+        action :foo do option "--bar" end
       end
 
       string.get_action(:foo).options.should =~ [:bar]
@@ -100,9 +89,9 @@ describe Puppet::String::Action do
     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
+          action :foo do option "--bar" end
+          action :baz do option "--bim" end
+          option "--quux"
         end
       end
 
@@ -125,24 +114,22 @@ describe Puppet::String::Action do
       end
     end
 
-    it "should fail when a duplicate option is added" do
-      expect {
-        Puppet::String.new(:action_level_options, '0.0.1') do
-          action :foo do
-            option :foo
-            option :foo
-          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
-      }.should raise_error ArgumentError, /foo duplicates an existing option/
+        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
+          option "--foo"
+          action :bar do option "--foo" end
         end
-      }.should raise_error ArgumentError, /duplicates an existing option .*action_level/i
+      }.should raise_error ArgumentError, /Option foo conflicts with existing option foo/i
     end
   end
 end
diff --git a/spec/unit/string/option_builder_spec.rb b/spec/unit/string/option_builder_spec.rb
index 685787808..9e913c29c 100644
--- a/spec/unit/string/option_builder_spec.rb
+++ b/spec/unit/string/option_builder_spec.rb
@@ -4,54 +4,26 @@ 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).
+    Puppet::String::OptionBuilder.build(string, "--foo").
       should be_an_instance_of Puppet::String::Option
   end
 
-  it "should set attributes during construction" do
-    # Walk all types, since at least one of them should be non-default...
-    Puppet::String::Option::Types.each do |type|
-      option = Puppet::String::OptionBuilder.build(string, :foo, :type => type)
-      option.should be_an_instance_of Puppet::String::Option
-      option.type.should == type
-    end
-  end
-
   describe "when using the DSL block" do
     it "should work with an empty block" do
-      option = Puppet::String::OptionBuilder.build(string, :foo) 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
 
-    describe "#type" do
-      Puppet::String::Option::Types.each do |valid|
-        it "should accept #{valid.inspect}" do
-          option = Puppet::String::OptionBuilder.build(string, :foo) do
-            type valid
-          end
-          option.should be_an_instance_of Puppet::String::Option
-        end
-
-        it "should accept #{valid.inspect} as a string" do
-          option = Puppet::String::OptionBuilder.build(string, :foo) do
-            type valid.to_s
-          end
-          option.should be_an_instance_of Puppet::String::Option
-        end
-
-        [:foo, nil, true, false, 12, '12', 'whatever', ::String, URI].each do |input|
-          it "should reject #{input.inspect}" do
-            expect {
-              Puppet::String::OptionBuilder.build(string, :foo) do
-                type input
-              end
-            }.should raise_error ArgumentError, /not a valid type/
-          end
-        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
index 9bb4309cd..fc7b8329b 100644
--- a/spec/unit/string/option_spec.rb
+++ b/spec/unit/string/option_spec.rb
@@ -3,59 +3,73 @@ 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 a name when created" do
+  it "also requires some declaration arguments when created" do
     expect { Puppet::String::Option.new(string) }.
-      should raise_error ArgumentError, /wrong number of arguments/
+      should raise_error ArgumentError, /No option declarations found/
   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
+  it "should infer the name from an optparse string" do
+    option = Puppet::String::Option.new(string, "--foo")
+    option.name.should == :foo
   end
 
-  describe "#to_s" do
-    it "should transform a symbol into a string" do
-      Puppet::String::Option.new(string, :foo).to_s.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 use - rather than _ to separate words" do
-      Puppet::String::Option.new(string, :foo_bar).to_s.should == "foo-bar"
-    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
 
-  describe "#type" do
-    Puppet::String::Option::Types.each do |type|
-      it "should accept #{type.inspect}" do
-        Puppet::String::Option.new(string, :foo, :type => type).
-          should be_an_instance_of Puppet::String::Option
-      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
 
-      it "should accept #{type.inspect} when given as a string" do
-        Puppet::String::Option.new(string, :foo, :type => type.to_s).
-          should be_an_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
 
-    [:foo, nil, true, false, 12, '12', 'whatever', ::String, URI].each do |input|
-      it "should reject #{input.inspect}" do
-        expect { Puppet::String::Option.new(string, :foo, :type => input) }.
-          should raise_error ArgumentError, /not a valid type/
-      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
-
-
-  # name         short  value          type
-  # ca-location         CA_LOCATION    string
-  # debug        d      ----           boolean
-  # verbose      v      ----           boolean
-  # terminus            TERMINUS       string
-  # format              FORMAT         symbol
-  # mode         r      RUNMODE        limited set of symbols
-  # server              URL            URL
 end
diff --git a/spec/unit/string_spec.rb b/spec/unit/string_spec.rb
index 577505186..7f7489e2e 100755
--- a/spec/unit/string_spec.rb
+++ b/spec/unit/string_spec.rb
@@ -82,66 +82,38 @@ describe Puppet::String do
 
   it "should be able to load all actions in all search paths"
 
-  describe "with string-level options" do
-    it "should support options without arguments" do
-      string = Puppet::String.new(:with_options, '0.0.1') do
-        option :foo
-      end
-      string.should be_an_instance_of Puppet::String
-      string.should be_option :foo
-    end
 
-    it "should support options with an empty block" do
-      string = Puppet::String.new(:with_options, '0.0.1') do
-        option :foo do
-          # this section deliberately left blank
-        end
-      end
-      string.should be_an_instance_of Puppet::String
-      string.should be_option :foo
-    end
-
-    it "should return all the string-level options" do
-      string = Puppet::String.new(:with_options, '0.0.1') do
-        option :foo
-        option :bar
-      end
-      string.options.should =~ [:foo, :bar]
+  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
+        option "--foo"
+        option "--bar"
         action :baz do
-          option :quux
+          option "--quux"
         end
       end
       string.options.should =~ [:foo, :bar]
     end
 
-    it "should fail when a duplicate option is added" do
-      expect {
-        Puppet::String.new(:action_level_options, '0.0.1') do
-          option :foo
-          option :foo
-        end
-      }.should raise_error ArgumentError, /option foo already defined for/i
-    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
+          action :bar do option "--foo" end
+          option "--foo"
         end
-      }.should raise_error ArgumentError, /foo already defined on action bar/i
+      }.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
+        action :foo do option "--quux" end
+        action :bar do option "--quux" end
       end
 
       string.get_action(:foo).options.should =~ [:quux]
@@ -152,9 +124,9 @@ describe Puppet::String do
   describe "with inherited options" do
     let :string do
       parent = Class.new(Puppet::String)
-      parent.option(:inherited, :type => :string)
+      parent.option("--inherited")
       string = parent.new(:example, '0.2.1')
-      string.option(:local)
+      string.option("--local")
       string
     end