7 files changed, 327 insertions, 65 deletions

diff --git a/lib/puppet/interface/action.rb b/lib/puppet/interface/action.rb
index f8eef69b1..114e5341b 100644
--- a/lib/puppet/interface/action.rb
+++ b/lib/puppet/interface/action.rb
@@ -1,15 +1,28 @@
-# -*- coding: utf-8 -*-
 require 'puppet/interface'
-require 'puppet/interface/option'
+require 'puppet/interface/documentation'
+require 'prettyprint'
 
 class Puppet::Interface::Action
+  extend Puppet::Interface::DocGen
+  include Puppet::Interface::FullDocs
+
   def initialize(face, name, attrs = {})
     raise "#{name.inspect} is an invalid action name" unless name.to_s =~ /^[a-z]\w*$/
     @face    = face
     @name    = name.to_sym
+
+    # The few bits of documentation we actually demand.  The default license
+    # is a favour to our end users; if you happen to get that in a core face
+    # report it as a bug, please. --daniel 2011-04-26
+    @authors = []
+    @license  = 'All Rights Reserved'
+
     attrs.each do |k, v| send("#{k}=", v) end
 
-    @options        = {}
+    # @options collects the added options in the order they're declared.
+    # @options_hash collects the options keyed by alias for quick lookups.
+    @options        = []
+    @options_hash   = {}
     @when_rendering = {}
   end
 
@@ -30,8 +43,32 @@ class Puppet::Interface::Action
     !!@default
   end
 
-  attr_accessor :summary
-
+  ########################################################################
+  # Documentation...
+  attr_doc :returns
+  def synopsis
+    output = PrettyPrint.format do |s|
+      s.text("puppet #{@face.name}")
+      s.text(" #{name}") unless default?
+      s.breakable
+
+      options.each do |option|
+        option = get_option(option)
+        wrap = option.required? ? %w{ < > } : %w{ [ ] }
+
+        s.group(0, *wrap) do
+          option.optparse.each do |item|
+            unless s.current_group.first?
+              s.breakable
+              s.text '|'
+              s.breakable
+            end
+            s.text item
+          end
+        end
+      end
+    end
+  end
 
   ########################################################################
   # Support for rendering formats and all.
@@ -39,8 +76,15 @@ class Puppet::Interface::Action
     unless type.is_a? Symbol
       raise ArgumentError, "The rendering format must be a symbol, not #{type.class.name}"
     end
-    return unless @when_rendering.has_key? type
-    return @when_rendering[type].bind(@face)
+    # Do we have a rendering hook for this name?
+    return @when_rendering[type].bind(@face) if @when_rendering.has_key? type
+
+    # How about by another name?
+    alt = type.to_s.sub(/^to_/, '').to_sym
+    return @when_rendering[alt].bind(@face) if @when_rendering.has_key? alt
+
+    # Guess not, nothing to run.
+    return nil
   end
   def set_rendering_method_for(type, proc)
     unless proc.is_a? Proc
@@ -83,18 +127,6 @@ class Puppet::Interface::Action
 
 
   ########################################################################
-  # Documentation stuff, whee!
-  attr_accessor :summary, :description
-  def summary=(value)
-    value = value.to_s
-    value =~ /\n/ and
-      raise ArgumentError, "Face summary should be a single line; put the long text in 'description' instead."
-
-    @summary = value
-  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 faces delegate to
@@ -148,11 +180,13 @@ class Puppet::Interface::Action
   # 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
+  attr_reader   :positional_arg_count
+  attr_accessor :when_invoked
   def when_invoked=(block)
 
     internal_name = "#{@name} implementation, required on Ruby 1.8".to_sym
 
-    arity = block.arity
+    arity = @positional_arg_count = block.arity
     if arity == 0 then
       # This will never fire on 1.8.7, which treats no arguments as "*args",
       # but will on 1.9.2, which treats it as "no arguments".  Which bites,
@@ -195,9 +229,11 @@ WRAPPER
     if @face.is_a?(Class)
       @face.class_eval do eval wrapper, nil, file, line end
       @face.define_method(internal_name, &block)
+      @when_invoked = @face.instance_method(name)
     else
       @face.instance_eval do eval wrapper, nil, file, line end
       @face.meta_def(internal_name, &block)
+      @when_invoked = @face.method(name).unbind
     end
   end
 
@@ -211,7 +247,8 @@ WRAPPER
     end
 
     option.aliases.each do |name|
-      @options[name] = option
+      @options << name
+      @options_hash[name] = option
     end
 
     option
@@ -223,15 +260,15 @@ WRAPPER
   end
 
   def option?(name)
-    @options.include? name.to_sym
+    @options_hash.include? name.to_sym
   end
 
   def options
-    (@options.keys + @face.options).sort
+    @face.options + @options
   end
 
   def get_option(name, with_inherited_options = true)
-    option = @options[name.to_sym]
+    option = @options_hash[name.to_sym]
     if option.nil? and with_inherited_options
       option = @face.get_option(name)
     end
@@ -239,12 +276,26 @@ WRAPPER
   end
 
   def validate_args(args)
+    # Check for multiple aliases for the same option...
+    args.last.keys.each do |name|
+      # #7290: If this isn't actually an option, ignore it for now.  We should
+      # probably fail, but that wasn't our API, and I don't want to perturb
+      # behaviour this late in the RC cycle. --daniel 2011-04-29
+      if option = get_option(name) then
+        overlap = (option.aliases & args.last.keys)
+        unless overlap.length == 1 then
+          raise ArgumentError, "Multiple aliases for the same option passed: #{overlap.join(', ')}"
+        end
+      end
+    end
+
+    # Check for missing mandatory options.
     required = options.map do |name|
       get_option(name)
     end.select(&:required?).collect(&:name) - args.last.keys
 
     return if required.empty?
-    raise ArgumentError, "missing required options (#{required.join(', ')})"
+    raise ArgumentError, "The following options are required: #{required.join(', ')}"
   end
 
   ########################################################################
diff --git a/lib/puppet/interface/action_builder.rb b/lib/puppet/interface/action_builder.rb
index fd8b0856f..ba5531f1d 100644
--- a/lib/puppet/interface/action_builder.rb
+++ b/lib/puppet/interface/action_builder.rb
@@ -9,13 +9,6 @@ class Puppet::Interface::ActionBuilder
     new(face, name, &block).action
   end
 
-  private
-  def initialize(face, name, &block)
-    @face   = face
-    @action = Puppet::Interface::Action.new(face, name)
-    instance_eval(&block)
-  end
-
   # Ideally the method we're defining here would be added to the action, and a
   # method on the face would defer to it, but we can't get scope correct, so
   # we stick with this. --daniel 2011-03-24
@@ -55,7 +48,7 @@ class Puppet::Interface::ActionBuilder
   def render_as(value = nil)
     value.nil? and raise ArgumentError, "You must give a rendering format to render_as"
 
-    formats = Puppet::Network::FormatHandler.formats << :for_humans
+    formats = Puppet::Network::FormatHandler.formats
     unless formats.include? value
       raise ArgumentError, "#{value.inspect} is not a valid rendering format: #{formats.sort.join(", ")}"
     end
@@ -66,18 +59,26 @@ class Puppet::Interface::ActionBuilder
   # Metaprogram the simple DSL from the target class.
   Puppet::Interface::Action.instance_methods.grep(/=$/).each do |setter|
     next if setter =~ /^=/
-    dsl = setter.sub(/=$/, '')
+    property = setter.sub(/=$/, '')
 
-    unless private_instance_methods.include? dsl
+    unless public_instance_methods.include? property
       # Using eval because the argument handling semantics are less awful than
       # when we use the define_method/block version.  The later warns on older
       # Ruby versions if you pass the wrong number of arguments, but carries
       # on, which is totally not what we want. --daniel 2011-04-18
-      eval <<METHOD
-def #{dsl}(value)
-  @action.#{dsl} = value
-end
-METHOD
+      eval <<-METHOD
+        def #{property}(value)
+          @action.#{property} = value
+        end
+      METHOD
     end
   end
+
+  private
+  def initialize(face, name, &block)
+    @face   = face
+    @action = Puppet::Interface::Action.new(face, name)
+    instance_eval(&block)
+    @action.when_invoked or raise ArgumentError, "actions need to know what to do when_invoked; please add the block"
+  end
 end
diff --git a/lib/puppet/interface/action_manager.rb b/lib/puppet/interface/action_manager.rb
index 440be2d1b..c5eb8e08a 100644
--- a/lib/puppet/interface/action_manager.rb
+++ b/lib/puppet/interface/action_manager.rb
@@ -1,9 +1,11 @@
-require 'puppet/interface/action_builder'
+require 'puppet/interface/action'
 
 module Puppet::Interface::ActionManager
   # Declare that this app can take a specific action, and provide
   # the code to do so.
   def action(name, &block)
+    require 'puppet/interface/action_builder'
+
     @actions ||= {}
     @default_action ||= nil
     raise "Action #{name} already defined for #{self}" if action?(name)
diff --git a/lib/puppet/interface/documentation.rb b/lib/puppet/interface/documentation.rb
new file mode 100644
index 000000000..48e9a8b1a
--- /dev/null
+++ b/lib/puppet/interface/documentation.rb
@@ -0,0 +1,197 @@
+# This isn't usable outside Puppet::Interface; don't load it alone.
+class Puppet::Interface
+  module DocGen
+    def self.strip_whitespace(text)
+      text.gsub!(/[ \t\f]+$/, '')
+
+      # We need to identify an indent: the minimum number of whitespace
+      # characters at the start of any line in the text.
+      #
+      # Using split rather than each_line is because the later only takes a
+      # block on Ruby 1.8.5 / Centos, and we support that. --daniel 2011-05-03
+      indent = text.split(/\n/).map {|x| x.index(/[^\s]/) }.compact.min
+
+      if indent > 0 then
+        text.gsub!(/^[ \t\f]{0,#{indent}}/, '')
+      end
+
+      return text
+    end
+
+    # The documentation attributes all have some common behaviours; previously
+    # we open-coded them across the set of six things, but that seemed
+    # wasteful - especially given that they were literally the same, and had
+    # the same bug hidden in them.
+    #
+    # This feels a bit like overkill, but at least the common code is common
+    # now. --daniel 2011-04-29
+    def attr_doc(name, &validate)
+      # Now, which form of the setter do we want, validated or not?
+      get_arg = "value.to_s"
+      if validate
+        define_method(:"_validate_#{name}", validate)
+        get_arg = "_validate_#{name}(#{get_arg})"
+      end
+
+      # We use module_eval, which I don't like much, because we can't have an
+      # argument to a block with a default value in Ruby 1.8, and I don't like
+      # the side-effects (eg: no argument count validation) of using blocks
+      # without as metheds.  When we are 1.9 only (hah!) you can totally
+      # replace this with some up-and-up define_method. --daniel 2011-04-29
+      module_eval(<<-EOT, __FILE__, __LINE__ + 1)
+        def #{name}(value = nil)
+          self.#{name} = value unless value.nil?
+          @#{name}
+        end
+
+        def #{name}=(value)
+          @#{name} = Puppet::Interface::DocGen.strip_whitespace(#{get_arg})
+        end
+      EOT
+    end
+  end
+
+  module TinyDocs
+    extend Puppet::Interface::DocGen
+
+    attr_doc :summary do |value|
+      value =~ /\n/ and
+        raise ArgumentError, "Face summary should be a single line; put the long text in 'description' instead."
+      value
+    end
+
+    attr_doc :description
+  end
+
+  module FullDocs
+    extend Puppet::Interface::DocGen
+    include TinyDocs
+
+    attr_doc :examples
+    attr_doc :notes
+    attr_doc :license
+
+    attr_doc :short_description
+    def short_description(value = nil)
+      self.short_description = value unless value.nil?
+      if @short_description.nil? then
+        return nil if @description.nil?
+        lines = @description.split("\n")
+        grab  = [5, lines.index('') || 5].min
+        @short_description = lines[0, grab].join("\n")
+      end
+      @short_description
+    end
+
+    def author(value = nil)
+      unless value.nil? then
+        unless value.is_a? String
+          raise ArgumentError, 'author must be a string; use multiple statements for multiple authors'
+        end
+
+        if value =~ /\n/ then
+          raise ArgumentError, 'author should be a single line; use multiple statements for multiple authors'
+        end
+        @authors.push(Puppet::Interface::DocGen.strip_whitespace(value))
+      end
+      @authors.empty? ? nil : @authors.join("\n")
+    end
+    def authors
+      @authors
+    end
+    def author=(value)
+      if Array(value).any? {|x| x =~ /\n/ } then
+        raise ArgumentError, 'author should be a single line; use multiple statements'
+      end
+      @authors = Array(value).map{|x| Puppet::Interface::DocGen.strip_whitespace(x) }
+    end
+    alias :authors= :author=
+
+    def copyright(owner = nil, years = nil)
+      if years.nil? and not owner.nil? then
+        raise ArgumentError, 'copyright takes the owners names, then the years covered'
+      end
+      self.copyright_owner = owner unless owner.nil?
+      self.copyright_years = years unless years.nil?
+
+      if self.copyright_years or self.copyright_owner then
+        "Copyright #{self.copyright_years} by #{self.copyright_owner}"
+      else
+        "Unknown copyright owner and years."
+      end
+    end
+
+    attr_accessor :copyright_owner
+    def copyright_owner=(value)
+      case value
+      when String then @copyright_owner = value
+      when Array  then @copyright_owner = value.join(", ")
+      else
+        raise ArgumentError, "copyright owner must be a string or an array of strings"
+      end
+      @copyright_owner
+    end
+
+    attr_accessor :copyright_years
+    def copyright_years=(value)
+      years = munge_copyright_year value
+      years = (years.is_a?(Array) ? years : [years]).
+        sort_by do |x| x.is_a?(Range) ? x.first : x end
+
+      @copyright_years = years.map do |year|
+        if year.is_a? Range then
+          "#{year.first}-#{year.last}"
+        else
+          year
+        end
+      end.join(", ")
+    end
+
+    def munge_copyright_year(input)
+      case input
+      when Range then input
+      when Integer then
+        if input < 1970 then
+          fault = "before 1970"
+        elsif input > (future = Time.now.year + 2) then
+          fault = "after #{future}"
+        end
+        if fault then
+          raise ArgumentError, "copyright with a year #{fault} is very strange; did you accidentally add or subtract two years?"
+        end
+
+        input
+
+      when String then
+        input.strip.split(/,/).map do |part|
+          part = part.strip
+          if part =~ /^\d+$/ then
+            part.to_i
+          elsif found = part.split(/-/) then
+            unless found.length == 2 and found.all? {|x| x.strip =~ /^\d+$/ }
+              raise ArgumentError, "#{part.inspect} is not a good copyright year or range"
+            end
+            Range.new(found[0].to_i, found[1].to_i)
+          else
+            raise ArgumentError, "#{part.inspect} is not a good copyright year or range"
+          end
+        end
+
+      when Array then
+        result = []
+        input.each do |item|
+          item = munge_copyright_year item
+          if item.is_a? Array
+            result.concat item
+          else
+            result << item
+          end
+        end
+        result
+
+      else
+        raise ArgumentError, "#{input.inspect} is not a good copyright year, set, or range"
+      end
+    end
+  end
+end
diff --git a/lib/puppet/interface/face_collection.rb b/lib/puppet/interface/face_collection.rb
index 6e6afc545..12d3c56b1 100644
--- a/lib/puppet/interface/face_collection.rb
+++ b/lib/puppet/interface/face_collection.rb
@@ -1,4 +1,3 @@
-# -*- coding: utf-8 -*-
 require 'puppet/interface'
 
 module Puppet::Interface::FaceCollection
@@ -10,21 +9,12 @@ module Puppet::Interface::FaceCollection
     unless @loaded
       @loaded = true
       $LOAD_PATH.each do |dir|
-        next unless FileTest.directory?(dir)
-        Dir.chdir(dir) do
-          Dir.glob("puppet/face/*.rb").collect { |f| f.sub(/\.rb/, '') }.each do |file|
-            iname = file.sub(/\.rb/, '')
-            begin
-              require iname
-            rescue Exception => detail
-              puts detail.backtrace if Puppet[:trace]
-              raise "Could not load #{iname} from #{dir}/#{file}: #{detail}"
-            end
-          end
-        end
+        Dir.glob("#{dir}/puppet/face/*.rb").
+          collect {|f| File.basename(f, '.rb') }.
+          each {|name| self[name, :current] }
       end
     end
-    return @faces.keys.select {|name| @faces[name].length > 0 }
+    @faces.keys.select {|name| @faces[name].length > 0 }
   end
 
   def self.validate_version(version)
@@ -124,6 +114,10 @@ module Puppet::Interface::FaceCollection
     rescue LoadError => e
       raise unless e.message =~ %r{-- puppet/face/#{name}$}
       # ...guess we didn't find the file; return a much better problem.
+    rescue SyntaxError => e
+      raise unless e.message =~ %r{puppet/face/#{name}\.rb:\d+: }
+      Puppet.err "Failed to load face #{name}:\n#{e}"
+      # ...but we just carry on after complaining.
     end
 
     return get_face(name, version)
diff --git a/lib/puppet/interface/option.rb b/lib/puppet/interface/option.rb
index f4c56cb2c..b68bdeb12 100644
--- a/lib/puppet/interface/option.rb
+++ b/lib/puppet/interface/option.rb
@@ -1,4 +1,10 @@
+require 'puppet/interface'
+
 class Puppet::Interface::Option
+  include Puppet::Interface::TinyDocs
+  # For compatibility, deprecated, and should go fairly soon...
+  ['', '='].each { |x| alias :"desc#{x}" :"description#{x}" }
+
   def initialize(parent, *declaration, &block)
     @parent   = parent
     @optparse = []
@@ -78,7 +84,7 @@ class Puppet::Interface::Option
   end
 
   attr_reader   :parent, :name, :aliases, :optparse
-  attr_accessor :required, :desc
+  attr_accessor :required
 
   attr_accessor :before_action
   def before_action=(proc)
diff --git a/lib/puppet/interface/option_manager.rb b/lib/puppet/interface/option_manager.rb
index d42359c07..326a91d92 100644
--- a/lib/puppet/interface/option_manager.rb
+++ b/lib/puppet/interface/option_manager.rb
@@ -8,6 +8,11 @@ module Puppet::Interface::OptionManager
   end
 
   def add_option(option)
+    # @options collects the added options in the order they're declared.
+    # @options_hash collects the options keyed by alias for quick lookups.
+    @options      ||= []
+    @options_hash ||= {}
+
     option.aliases.each do |name|
       if conflict = get_option(name) then
         raise ArgumentError, "Option #{option} conflicts with existing option #{conflict}"
@@ -21,25 +26,30 @@ module Puppet::Interface::OptionManager
       end
     end
 
-    option.aliases.each { |name| @options[name] = option }
-    option
+    option.aliases.each do |name|
+      @options << name
+      @options_hash[name] = option
+    end
+
+    return option
   end
 
   def options
-    @options ||= {}
-    result = @options.keys
+    result = (@options ||= [])
 
     if self.is_a?(Class) and superclass.respond_to?(:options)
-      result += superclass.options
+      result = superclass.options + result
     elsif self.class.respond_to?(:options)
-      result += self.class.options
+      result = self.class.options + result
     end
-    result.sort
+
+    return result
   end
 
   def get_option(name, with_inherited_options = true)
-    @options ||= {}
-    result = @options[name.to_sym]
+    @options_hash ||= {}
+
+    result = @options_hash[name.to_sym]
     if result.nil? and with_inherited_options then
       if self.is_a?(Class) and superclass.respond_to?(:get_option)
         result = superclass.get_option(name)
@@ -47,6 +57,7 @@ module Puppet::Interface::OptionManager
         result = self.class.get_option(name)
       end
     end
+
     return result
   end