Please review pull request #722: (#14200) Fix help face opened by (cprice-puppet)
Description:
This commit does the following:
- Fix the help face so that--in the case where there exists both an App and a Face by the same name, and the App is not just a stub for the Face--help gives preference to the App instead of the Face. The help face (as a command line tool) is mostly going to be used to help users navigate the other command line tools, so, e.g., "puppet help resource" should show help for the same tool that you will get by running "puppet resource". This was not the case prior to this commit.
- Get rid of the separation between faces and applications in the help screen... making this distinction seems more confusing than helpful to the users, and exposes implementation details that users should not need to be concerned with.
- Change all class names for Applications to conform to the convention of "foo_bar" file names mapping to "FooBar" class names.
- Opened: Thu Apr 26 21:10:57 UTC 2012
- Based on: puppetlabs:master (d63ca26332d3f93e7cdd1b357e1e246755b21bff)
- Requested merge: cprice-puppet:bug/master/14200-help-face-bad-docs (20410528e2c2c05e37bfad49641b13087ae38f2f)
Diff follows:
diff --git a/lib/puppet/application.rb b/lib/puppet/application.rb
index b009902..f5121c2 100644
--- a/lib/puppet/application.rb
+++ b/lib/puppet/application.rb
@@ -1,5 +1,6 @@
require 'optparse'
require 'puppet/util/plugins'
+require 'puppet/util/constant_inflector'
require 'puppet/error'
# This class handles all the aspects of a Puppet application/executable
@@ -219,7 +220,7 @@ def option_parser_commands
end
def find(name)
- klass = name.to_s.capitalize
+ klass = Puppet::Util::ConstantInflector.file2constant(name.to_s)
begin
require ::File.join('puppet', 'application', name.to_s.downcase)
diff --git a/lib/puppet/application/certificate_request.rb b/lib/puppet/application/certificate_request.rb
index 1b1b083..8a26703 100644
--- a/lib/puppet/application/certificate_request.rb
+++ b/lib/puppet/application/certificate_request.rb
@@ -1,4 +1,4 @@
require 'puppet/application/indirection_base'
-class Puppet::Application::Certificate_request < Puppet::Application::IndirectionBase
+class Puppet::Application::CertificateRequest < Puppet::Application::IndirectionBase
end
diff --git a/lib/puppet/application/certificate_revocation_list.rb b/lib/puppet/application/certificate_revocation_list.rb
index 60b9d97..ee10775 100644
--- a/lib/puppet/application/certificate_revocation_list.rb
+++ b/lib/puppet/application/certificate_revocation_list.rb
@@ -1,4 +1,4 @@
require 'puppet/application/indirection_base'
-class Puppet::Application::Certificate_revocation_list < Puppet::Application::IndirectionBase
+class Puppet::Application::CertificateRevocationList < Puppet::Application::IndirectionBase
end
diff --git a/lib/puppet/application/face_base.rb b/lib/puppet/application/face_base.rb
index b8ecc9c..c85b2ce 100644
--- a/lib/puppet/application/face_base.rb
+++ b/lib/puppet/application/face_base.rb
@@ -69,7 +69,7 @@ def parse_options
# REVISIT: 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
+ @type = Puppet::Util::ConstantInflector.constant2file(self.class.name.to_s.sub(/.+:/, '')).to_sym
@face = Puppet::Face[@type, :current]
# Now, walk the command line and identify the action. We skip over
diff --git a/lib/puppet/application/instrumentation_data.rb b/lib/puppet/application/instrumentation_data.rb
index e4f86f1..647fc36 100644
--- a/lib/puppet/application/instrumentation_data.rb
+++ b/lib/puppet/application/instrumentation_data.rb
@@ -1,4 +1,4 @@
require 'puppet/application/indirection_base'
-class Puppet::Application::Instrumentation_data < Puppet::Application::IndirectionBase
+class Puppet::Application::InstrumentationData < Puppet::Application::IndirectionBase
end
diff --git a/lib/puppet/application/instrumentation_listener.rb b/lib/puppet/application/instrumentation_listener.rb
index 64029b5..216e952 100644
--- a/lib/puppet/application/instrumentation_listener.rb
+++ b/lib/puppet/application/instrumentation_listener.rb
@@ -1,4 +1,4 @@
require 'puppet/application/indirection_base'
-class Puppet::Application::Instrumentation_listener < Puppet::Application::IndirectionBase
+class Puppet::Application::InstrumentationListener < Puppet::Application::IndirectionBase
end
diff --git a/lib/puppet/application/instrumentation_probe.rb b/lib/puppet/application/instrumentation_probe.rb
index b31f95c..8f808c6 100644
--- a/lib/puppet/application/instrumentation_probe.rb
+++ b/lib/puppet/application/instrumentation_probe.rb
@@ -1,4 +1,4 @@
require 'puppet/application/indirection_base'
-class Puppet::Application::Instrumentation_probe < Puppet::Application::IndirectionBase
+class Puppet::Application::InstrumentationProbe < Puppet::Application::IndirectionBase
end
diff --git a/lib/puppet/application/resource_type.rb b/lib/puppet/application/resource_type.rb
index 5959426..7842257 100644
--- a/lib/puppet/application/resource_type.rb
+++ b/lib/puppet/application/resource_type.rb
@@ -1,4 +1,4 @@
require 'puppet/application/indirection_base'
-class Puppet::Application::Resource_type < Puppet::Application::IndirectionBase
+class Puppet::Application::ResourceType < Puppet::Application::IndirectionBase
end
diff --git a/lib/puppet/application/secret_agent.rb b/lib/puppet/application/secret_agent.rb
index 6bdbc92..6d11be4 100644
--- a/lib/puppet/application/secret_agent.rb
+++ b/lib/puppet/application/secret_agent.rb
@@ -1,6 +1,6 @@
require 'puppet/application/face_base'
require 'puppet/face'
-class Puppet::Application::Secret_agent < Puppet::Application::FaceBase
+class Puppet::Application::SecretAgent < Puppet::Application::FaceBase
run_mode :agent
end
diff --git a/lib/puppet/face/help.rb b/lib/puppet/face/help.rb
index 4a16a0d..2055f83 100644
--- a/lib/puppet/face/help.rb
+++ b/lib/puppet/face/help.rb
@@ -1,5 +1,7 @@
require 'puppet/face'
+require 'puppet/application/face_base'
require 'puppet/util/command_line'
+require 'puppet/util/constant_inflector'
require 'pathname'
require 'erb'
@@ -100,17 +102,38 @@ def erb(name)
return erb
end
+ # Return a list of applications that are not simply just stubs for Faces.
def legacy_applications
- # The list of applications, less those that are duplicated as a face.
Puppet::Util::CommandLine.available_subcommands.reject do |appname|
- Puppet::Face.face? appname.to_sym, :current or
- # ...this is a nasty way to exclude non-applications. :(
- %w{face_base indirection_base}.include? appname
+ (is_face_app?(appname)) or (exclude_from_docs?(appname))
end.sort
end
+ # Return a list of all applications (both legacy and Face applications), along with a summary
+ # of their functionality.
+ # @returns [Array] An Array of Arrays. The outer array contains one entry per application; each
+ # element in the outer array is a pair whose first element is a String containing the application
+ # name, and whose second element is a String containing the summary for that application.
+ def all_application_summaries()
+ Puppet::Util::CommandLine.available_subcommands.sort.inject([]) do |result, appname|
+ next result if exclude_from_docs?(appname)
+
+ if (is_face_app?(appname))
+ face = Puppet::Face[appname, :current]
+ result << [appname, face.summary]
+ else
+ result << [appname, horribly_extract_summary_from(appname)]
+ end
+ end
+ end
+
def horribly_extract_summary_from(appname)
begin
+ # it sucks that this 'require' is necessary, and it sucks even more that we are
+ # doing it in two different places in this class (#horribly_extract_summary_from,
+ # #is_face_app?). However, we can take some solace in the fact that ruby will
+ # at least recognize that it's already done a 'require' for any individual app
+ # and basically treat it as a no-op if we try to 'require' it twice.
require "puppet/application/#{appname}"
help = Puppet::Application[appname].help.split("\n")
# Now we find the line with our summary, extract it, and return it. This
@@ -128,4 +151,34 @@ def horribly_extract_summary_from(appname)
end
return ''
end
+ # This should absolutely be a private method, but for some reason it appears
+ # that you can't use the 'private' keyword inside of a Face definition.
+ # See #14205.
+ #private :horribly_extract_summary_from
+
+ def exclude_from_docs?(appname)
+ %w{face_base indirection_base}.include? appname
+ end
+ # This should absolutely be a private method, but for some reason it appears
+ # that you can't use the 'private' keyword inside of a Face definition.
+ # See #14205.
+ #private :exclude_from_docs?
+
+ def is_face_app?(appname)
+ # it sucks that this 'require' is necessary, and it sucks even more that we are
+ # doing it in two different places in this class (#horribly_extract_summary_from,
+ # #is_face_app?). However, we can take some solace in the fact that ruby will
+ # at least recognize that it's already done a 'require' for any individual app
+ # and basically treat it as a no-op if we try to 'require' it twice.
+ require "puppet/application/#{appname}"
+ # Would much rather use "const_get" than "eval" here, but for some reason it is not available.
+ # See #14205.
+ clazz = eval("Puppet::Application::#{Puppet::Util::ConstantInflector.file2constant(appname)}")
+ clazz.ancestors.include?(Puppet::Application::FaceBase)
+ end
+ # This should probably be a private method, but for some reason it appears
+ # that you can't use the 'private' keyword inside of a Face definition.
+ # See #14205.
+ #private :is_face_app?
+
end
diff --git a/lib/puppet/face/help/global.erb b/lib/puppet/face/help/global.erb
index c5a9ec9..800afff 100644
--- a/lib/puppet/face/help/global.erb
+++ b/lib/puppet/face/help/global.erb
@@ -1,18 +1,14 @@
Usage: puppet <subcommand> [options] <action> [options]
-Available subcommands, from Puppet Faces:
-<% Puppet::Face.faces.sort.each do |name|
- face = Puppet::Face[name, :current] -%>
- <%= face.name.to_s.ljust(16) %> <%= face.summary %>
-<% end -%>
-
-<% unless legacy_applications.empty? then # great victory when this is true! -%>
-Available applications, soon to be ported to Faces:
-<% legacy_applications.each do |appname|
- summary = horribly_extract_summary_from appname -%>
+Available subcommands:
+ <%# NOTE: this is probably not a good long-term solution for this. We're only iterating over
+ applications to find the list of things we need to show help for... this works for now
+ because faces can't be run without an application stub. However, when #6753 is resolved,
+ all of the application stubs for faces will go away, and this will need to be updated
+ to reflect that. --cprice 2012-04-26 %>
+<% all_application_summaries.each do |appname, summary| -%>
<%= appname.to_s.ljust(16) %> <%= summary %>
-<% end
- end -%>
+<% end -%>
See 'puppet help <subcommand> <action>' for help on a specific subcommand action.
See 'puppet help <subcommand>' for help on a specific subcommand.
diff --git a/lib/puppet/util/command_line.rb b/lib/puppet/util/command_line.rb
index 4200f7a..8809901 100644
--- a/lib/puppet/util/command_line.rb
+++ b/lib/puppet/util/command_line.rb
@@ -133,7 +133,7 @@ def execute
# (i.e.. so that we can load faces). Longer-term, use the autoloader. See comments in
# #available_subcommands method above. --cprice 2012-03-06
app = Puppet::Application.find(subcommand_name).new(self)
- Puppet::Plugins.on_application_initialization(:appliation_object => self)
+ Puppet::Plugins.on_application_initialization(:application_object => self)
app.run
elsif ! execute_external_subcommand then
diff --git a/lib/puppet/util/constant_inflector.rb b/lib/puppet/util/constant_inflector.rb
index 8f93e32..128bbc9 100644
--- a/lib/puppet/util/constant_inflector.rb
+++ b/lib/puppet/util/constant_inflector.rb
@@ -1,6 +1,8 @@
# Created on 2008-02-12
# Copyright Luke Kanies
+# NOTE: I think it might be worth considering moving these methods directly into Puppet::Util.
+
# A common module for converting between constants and
# file names.
module Puppet::Util::ConstantInflector
@@ -8,8 +10,10 @@ def file2constant(file)
# LAK:NOTE See http://snurl.com/21zf8 [groups_google_com]
x = file.split("/").collect { |name| name.capitalize }.join("::").gsub(/_+(.)/) { |term| $1.capitalize }
end
+ module_function :file2constant
def constant2file(constant)
constant.to_s.gsub(/([a-z])([A-Z])/) { |term| $1 + "_#{$2}" }.gsub("::", "/").downcase
end
+ module_function :constant2file
end
diff --git a/spec/unit/application/indirection_base_spec.rb b/spec/unit/application/indirection_base_spec.rb
index 8a5eee2..5d56603 100755
--- a/spec/unit/application/indirection_base_spec.rb
+++ b/spec/unit/application/indirection_base_spec.rb
@@ -8,7 +8,7 @@
class Puppet::Application::TestIndirection < Puppet::Application::IndirectionBase
end
-face = Puppet::Indirector::Face.define(:testindirection, '0.0.1') do
+face = Puppet::Indirector::Face.define(:test_indirection, '0.0.1') do
summary "fake summary"
copyright "Puppet Labs", 2011
license "Apache 2 license; see COPYING"
@@ -26,10 +26,14 @@ class Puppet::Application::TestIndirection < Puppet::Application::IndirectionBas
# 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 = stub_everything("test indirection terminus")
- terminus.stubs(:name).returns(:testindirection)
+ terminus.stubs(:name).returns(:test_indirection)
+
+ # This is necessary because Instrumentation tickles indirection, which
+ # messes up our expectations.
+ Puppet::Util::Instrumentation.stubs(:init)
Puppet::Indirector::Indirection.expects(:instance).
- with(:testindirection).returns(terminus)
+ with(:test_indirection).returns(terminus)
subject.command_line.instance_variable_set('@args', %w{--terminus foo save bar})
@@ -37,6 +41,8 @@ class Puppet::Application::TestIndirection < Puppet::Application::IndirectionBas
$stderr.stubs(:puts)
Puppet.stubs(:err)
- expect { subject.run }.to exit_with 0
+ expect {
+ subject.run
+ }.to exit_with 0
end
end
diff --git a/spec/unit/face/help_spec.rb b/spec/unit/face/help_spec.rb
index e5b6942..2c6582d 100755
--- a/spec/unit/face/help_spec.rb
+++ b/spec/unit/face/help_spec.rb
@@ -12,7 +12,9 @@
end
it "should accept a call with no arguments" do
- expect { subject.help() }.should_not raise_error
+ expect {
+ subject.help()
+ }.should_not raise_error
end
it "should accept a face name" do
@@ -62,8 +64,16 @@
end
end
- it "should list all faces" do
- Puppet::Face.faces.each do |name|
+ it "should list all faces which are runnable from the command line" do
+ help_face = Puppet::Face[:help, :current]
+ # The main purpose of the help face is to provide documentation for
+ # command line users. It shouldn't show documentation for faces
+ # that can't be run from the command line, so, rather than iterating
+ # over all available faces, we need to iterate over the subcommands
+ # that are available from the command line.
+ Puppet::Util::CommandLine.available_subcommands.each do |name|
+ next unless help_face.is_face_app?(name)
+ next if help_face.exclude_from_docs?(name)
face = Puppet::Face[name, :current]
summary = face.summary
diff --git a/spec/unit/util/constant_inflector_spec.rb b/spec/unit/util/constant_inflector_spec.rb
index 88c1d9a..de0ba7b 100755
--- a/spec/unit/util/constant_inflector_spec.rb
+++ b/spec/unit/util/constant_inflector_spec.rb
@@ -4,63 +4,53 @@
require 'puppet/util/constant_inflector'
describe Puppet::Util::ConstantInflector, "when converting file names to constants" do
- before do
- @inflector = Object.new
- @inflector.extend(Puppet::Util::ConstantInflector)
- end
-
it "should capitalize terms" do
- @inflector.file2constant("file").should == "File"
+ subject.file2constant("file").should == "File"
end
it "should switch all '/' characters to double colons" do
- @inflector.file2constant("file/other").should == "File::Other"
+ subject.file2constant("file/other").should == "File::Other"
end
it "should remove underscores and capitalize the proceeding letter" do
- @inflector.file2constant("file_other").should == "FileOther"
+ subject.file2constant("file_other").should == "FileOther"
end
it "should correctly replace as many underscores as exist in the file name" do
- @inflector.file2constant("two_under_scores/with_some_more_underscores").should == "TwoUnderScores::WithSomeMoreUnderscores"
+ subject.file2constant("two_under_scores/with_some_more_underscores").should == "TwoUnderScores::WithSomeMoreUnderscores"
end
it "should collapse multiple underscores" do
- @inflector.file2constant("many___scores").should == "ManyScores"
+ subject.file2constant("many___scores").should == "ManyScores"
end
it "should correctly handle file names deeper than two directories" do
- @inflector.file2constant("one_two/three_four/five_six").should == "OneTwo::ThreeFour::FiveSix"
+ subject.file2constant("one_two/three_four/five_six").should == "OneTwo::ThreeFour::FiveSix"
end
end
describe Puppet::Util::ConstantInflector, "when converting constnats to file names" do
- before do
- @inflector = Object.new
- @inflector.extend(Puppet::Util::ConstantInflector)
- end
-
it "should convert them to a string if necessary" do
- @inflector.constant2file(Puppet::Util::ConstantInflector).should be_instance_of(String)
+ subject.constant2file(Puppet::Util::ConstantInflector).should be_instance_of(String)
end
it "should accept string inputs" do
- @inflector.constant2file("Puppet::Util::ConstantInflector").should be_instance_of(String)
+ subject.constant2file("Puppet::Util::ConstantInflector").should be_instance_of(String)
end
it "should downcase all terms" do
- @inflector.constant2file("Puppet").should == "puppet"
+ subject.constant2file("Puppet").should == "puppet"
end
it "should convert '::' to '/'" do
- @inflector.constant2file("Puppet::Util::Constant").should == "puppet/util/constant"
+ subject.constant2file("Puppet::Util::Constant").should == "puppet/util/constant"
end
it "should convert mid-word capitalization to an underscore" do
- @inflector.constant2file("OneTwo::ThreeFour").should == "one_two/three_four"
+ subject.constant2file("OneTwo::ThreeFour").should == "one_two/three_four"
end
it "should correctly handle constants with more than two parts" do
- @inflector.constant2file("OneTwoThree::FourFiveSixSeven").should == "one_two_three/four_five_six_seven"
+ subject.constant2file("OneTwoThree::FourFiveSixSeven").should == "one_two_three/four_five_six_seven"
end
end
-- You received this message because you are subscribed to the Google Groups "Puppet Developers" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to [email protected].
For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
