(PDOC-3) Refactor face and remove modules action

Refactor the code in the strings face and extract much of the functionality
into a separate library in order to make the code more readable and
sustainable. Additionally, remove the `modules` action from the face
since it does not have a clear use case currently. However since the `server`
action depends on it, and it may be helpful to add other features in the future,
extract the functionality previously contained in `modules` into a helper
method.

lib/puppet/face/strings.rb

@@ -1,37 +1,11 @@
 require 'puppet/face'
+require 'puppetx/puppetlabs/strings/actions'
+
+include Puppetx::PuppetLabs::Strings::Actions
 
 Puppet::Face.define(:strings, '0.0.1') do
   summary "Generate Puppet documentation with YARD."
 
-  def check_required_features
-    unless Puppet.features.yard?
-      raise RuntimeError, "The 'yard' gem must be installed in order to use this face."
-    end
-
-    unless Puppet.features.rgen?
-      raise RuntimeError, "The 'rgen' gem must be installed in order to use this face."
-    end
-
-    if RUBY_VERSION.match(/^1\.8/)
-      raise RuntimeError, "This face requires Ruby 1.9 or greater."
-    end
-  end
-
-  # Maps things like the Puppet `--debug` flag to YARD options.
-  def merge_puppet_args!(yard_args)
-    yard_args.unshift '--debug'     if Puppet[:debug]
-    yard_args.unshift '--backtrace' if Puppet[:trace]
-
-    yard_args
-  end
-
-  # A list of globs that generates the default list of module files from which
-  # documentation can be extracted.
-  #
-  # TODO: It would be awesome if we could somehow override/append to the
-  # default file list that YARD uses. Consider an upstream PR for this.
-  MODULE_SOURCEFILES = ['manifests/**/*.pp', 'lib/**/*.rb']
-
   action(:yardoc) do
     default
 
@@ -40,6 +14,7 @@ Puppet::Face.define(:strings, '0.0.1') do
 
     when_invoked do |*args|
       check_required_features
+      require 'puppetx/puppetlabs/strings/yard/plugin'
 
       # The last element of the argument array should be the options hash.
       #
@@ -52,12 +27,8 @@ Puppet::Face.define(:strings, '0.0.1') do
       # For now, assume the remaining positional args are a list of manifest
       # and ruby files to parse.
       yard_args = (args.empty? ? MODULE_SOURCEFILES : args)
-      merge_puppet_args!(yard_args)
 
-      require 'puppetx/puppetlabs/strings/yard/plugin'
+      generate_documentation(*yard_args)
-
-      # Hand off to YARD for further processing.
-      YARD::CLI::Yardoc.run(*yard_args)
     end
   end
 
@@ -65,40 +36,6 @@ Puppet::Face.define(:strings, '0.0.1') do
   # (.yardoc directories) for Ruby Gems. Currently lacks the fine-grained
   # control over where these indicies are created and just dumps them in the
   # module roots.
-  action(:modules) do
-    summary "Generate YARD indices for a list of modules."
-    arguments "[module-name ...]"
-
-    when_invoked do |*args|
-      check_required_features
-      require 'puppetx/puppetlabs/strings/yard/plugin'
-      opts = args.pop
-
-      # NOTE: The retrun value of the `module` Face seems to have changed in
-      # 3.6.x. This part of the code will blow up if run under an earlier
-      # version of Puppet.
-      modules = Puppet::Face[:module, :current].list
-      module_list = modules[:modules_by_path].values.flatten
-
-      # TODO: Can use select! if Ruby 1.8.7 support is dropped.
-      module_list = module_list.select {|m| args.include? m.name} unless args.empty?
-
-      # Invoke `yardoc` with -n so that it doesn't generate any HTML output but
-      # does build a `.yardoc` index that other tools can generate output from.
-      yard_args = %w[--no-stats -n] + MODULE_SOURCEFILES
-      merge_puppet_args!(yard_args)
-
-      module_list.each do |m|
-        Dir.chdir(m.path) do
-          YARD::CLI::Yardoc.run(*yard_args)
-
-          # Cear the global Registry so that objects from one module don't
-          # bleed into the next.
-          YARD::Registry.clear
-        end
-      end
-    end
-  end
 
   action(:server) do
     summary "Serve YARD documentation for modules."
@@ -108,16 +45,17 @@ Puppet::Face.define(:strings, '0.0.1') do
       require 'puppetx/puppetlabs/strings/yard/plugin'
       opts = args.pop
 
+      module_names = args
+
       # FIXME: This is pretty inefficient as it forcibly re-generates the YARD
       # indicies each time the server is started. However, it ensures things are
       # generated properly.
-      module_list = Puppet::Face[:strings, :current].modules
+      module_list = index_documentation_for_modules(module_names)
 
-      module_tuples = module_list.map do |mod|
+      module_tuples = generate_module_tuples(module_list)
-        name = (mod.forge_name || mod.name).gsub('/', '-')
-        yard_index = File.join(mod.path, '.yardoc')
 
-        [name, yard_index]
+      module_tuples.map! do |mod|
+        [mod[:name], mod[:index_path]]
       end
 
       # The `-m` flag means a list of name/path pairs will follow. The name is
@@ -126,7 +64,8 @@ Puppet::Face.define(:strings, '0.0.1') do
       yard_args = %w[-m -q] + module_tuples.flatten
       merge_puppet_args!(yard_args)
 
-      YARD::CLI::Server.run(*yard_args)
+      serve_documentation(*yard_args)
     end
   end
 end
+

lib/puppetx/puppetlabs/strings/actions.rb

@@ -0,0 +1,86 @@
+require 'puppetx/puppetlabs/strings'
+
+module Puppetx::PuppetLabs::Strings::Actions
+
+  ModuleIndex = Struct.new(:name, :index_path)
+
+  # A list of globs that generates the default list of module files from which
+  # documentation can be extracted.
+  #
+  # TODO: It would be awesome if we could somehow override/append to the
+  # default file list that YARD uses. Consider an upstream PR for this.
+  MODULE_SOURCEFILES = ['manifests/**/*.pp', 'lib/**/*.rb']
+
+  def check_required_features
+    unless Puppet.features.yard?
+      raise RuntimeError, "The 'yard' gem must be installed in order to use this face."
+    end
+
+    unless Puppet.features.rgen?
+      raise RuntimeError, "The 'rgen' gem must be installed in order to use this face."
+    end
+
+    if RUBY_VERSION.match(/^1\.8/)
+      raise RuntimeError, "This face requires Ruby 1.9 or greater."
+    end
+  end
+
+  # Maps things like the Puppet `--debug` flag to YARD options.
+  def merge_puppet_args!(yard_args)
+    yard_args.unshift '--debug'     if Puppet[:debug]
+    yard_args.unshift '--backtrace' if Puppet[:trace]
+
+    yard_args
+  end
+
+  def index_documentation_for_modules(module_names)
+    check_required_features
+    require 'puppetx/puppetlabs/strings/yard/plugin'
+
+    # NOTE: The retrun value of the `module` Face seems to have changed in
+    # 3.6.x. This part of the code will blow up if run under an earlier
+    # version of Puppet.
+    modules = Puppet::Face[:module, :current].list
+    module_list = modules[:modules_by_path].values.flatten
+
+    # TODO: Can use select! if Ruby 1.8.7 support is dropped.
+    module_list.select! {|m| module_names.include? m.name} unless module_names.empty?
+
+    # Invoke `yardoc` with -n so that it doesn't generate any HTML output but
+    # does build a `.yardoc` index that other tools can generate output from.
+    yard_args = %w[--no-stats -n] + MODULE_SOURCEFILES
+    merge_puppet_args!(yard_args)
+
+    module_list.each do |m|
+      Dir.chdir(m.path) do
+        YARD::CLI::Yardoc.run(*yard_args)
+
+        # Cear the global Registry so that objects from one module don't
+        # bleed into the next.
+        YARD::Registry.clear
+      end
+    end
+  end
+
+  def generate_module_tuples(module_list)
+    module_list.map do |mod|
+      name = (mod.forge_name || mod.name).gsub('/', '-')
+      yard_index = File.join(mod.path, '.yardoc')
+
+      #[name, yard_index]
+      ModuleIndex.new(name, yard_index)
+    end
+  end
+
+  def serve_documentation(*yard_args)
+    merge_puppet_args!(yard_args)
+    # Hand off to YARD for further processing.
+    YARD::CLI::Server.run(*yard_args)
+  end
+
+  def generate_documentation(*yard_args)
+    merge_puppet_args!(yard_args)
+    # Hand off to YARD for further processing.
+    YARD::CLI::Yardoc.run(*yard_args)
+  end
+end

spec/unit/puppet/face_spec.rb

@@ -67,22 +67,6 @@ describe Puppet::Face do
     end
   end
 
-  describe "modules action" do
-    it "should raise an error if yard is absent" do
-      Puppet.features.stubs(:yard?).returns(false)
-      expect{Puppet::Face[:strings, :current].modules}.to raise_error(RuntimeError, "The 'yard' gem must be installed in order to use this face.")
-    end
-
-    it "should raise an error if rgen is absent" do
-      Puppet.features.stubs(:rgen?).returns(false)
-      expect{Puppet::Face[:strings, :current].modules}.to raise_error(RuntimeError, "The 'rgen' gem must be installed in order to use this face.")
-    end
-
-    it "should raise an error if the Ruby version is less than 1.9", :if => RUBY_VERSION.match(/^1\.8/) do
-      expect{Puppet::Face[:strings, :current].modules}.to raise_error(RuntimeError, "This face requires Ruby 1.9 or greater.")
-    end
-  end
-
   describe "server action" do
     it "should raise an error if yard is absent" do
       Puppet.features.stubs(:yard?).returns(false)