From 49171c92b9fadc54dbe840e916b628645542490b Mon Sep 17 00:00:00 2001 From: Ian Kronquist Date: Wed, 24 Jun 2015 12:36:13 -0700 Subject: [PATCH 1/4] (PDOC-37) Warn if parameter names don't match docs Print the name of the parameter, the file, and the line number to stderr if the parameter name does not match the name specified in the docs. In order for this to be useful we need to present the user with the file name and line number of the relevant parameters. This information needs to be extracted from the code object and passed to the extract_param_details method. --- .../yard/templates/default/definedtype/setup.rb | 2 +- .../templates/default/puppetnamespace/setup.rb | 2 +- .../yard/templates/default/template_helper.rb | 15 +++++++++++---- 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/lib/puppet_x/puppetlabs/strings/yard/templates/default/definedtype/setup.rb b/lib/puppet_x/puppetlabs/strings/yard/templates/default/definedtype/setup.rb index 39ae076..31d54fd 100644 --- a/lib/puppet_x/puppetlabs/strings/yard/templates/default/definedtype/setup.rb +++ b/lib/puppet_x/puppetlabs/strings/yard/templates/default/definedtype/setup.rb @@ -18,7 +18,7 @@ def parameter_details @param_details = [] - @param_details = @template_helper.extract_param_details(params, param_tags, true) + @param_details = @template_helper.extract_param_details(params, param_tags, object.files, true) erb(:parameter_details) end diff --git a/lib/puppet_x/puppetlabs/strings/yard/templates/default/puppetnamespace/setup.rb b/lib/puppet_x/puppetlabs/strings/yard/templates/default/puppetnamespace/setup.rb index 054cee2..5238f64 100644 --- a/lib/puppet_x/puppetlabs/strings/yard/templates/default/puppetnamespace/setup.rb +++ b/lib/puppet_x/puppetlabs/strings/yard/templates/default/puppetnamespace/setup.rb @@ -75,7 +75,7 @@ def method_details_list # Convert the matched string into an array of strings params = parameters.nil? ? nil : parameters[1].split(/\s*,\s*/) - param_details = @template_helper.extract_param_details(params, param_tags) unless params.nil? + param_details = @template_helper.extract_param_details(params, param_tags, object.files) unless params.nil? else param_details = @template_helper.comment_only_param_details(param_tags) end diff --git a/lib/puppet_x/puppetlabs/strings/yard/templates/default/template_helper.rb b/lib/puppet_x/puppetlabs/strings/yard/templates/default/template_helper.rb index 40cf94e..030fd8f 100644 --- a/lib/puppet_x/puppetlabs/strings/yard/templates/default/template_helper.rb +++ b/lib/puppet_x/puppetlabs/strings/yard/templates/default/template_helper.rb @@ -1,7 +1,8 @@ +require "puppet" + # A class containing helper methods to aid in the extraction of relevant data # from comments and YARD tags class TemplateHelper - # Extracts data from comments which include the supported YARD tags def extract_tag_data(object) examples = Hash.new @@ -35,13 +36,12 @@ class TemplateHelper # :desc => [String] The description provided in the comment # :types => [Array] The parameter type(s) specified in the comment # :exists => [Boolean] True only if the parameter exists in the documented logic and not just in a comment} - def extract_param_details(parameters, tags_hash, fq_name = false) + def extract_param_details(parameters, tags_hash, locations, fq_name = false) parameter_info = [] - # Extract the information for parameters that actually exist + # Extract the information for parameters that exist # as opposed to parameters that are defined only in the comments parameters.each do |param| - if fq_name param_name = param[0] fully_qualified_name = param[1] @@ -73,6 +73,13 @@ class TemplateHelper end if !param_exists parameter_info.push({:name => tag.name, :desc => tag.text, :types => tag.types, :exists? => false}) + if locations.length >= 1 and locations[0].length == 2 + file_name = locations[0][0] + line_number = locations[0][1] + $stderr.puts "[warn]: The parameter #{tag.name} is documented, but doesn't exist in your code, in file #{file_name} near line #{line_number}" + else + $stderr.puts "[warn]: The parameter #{tag.name} is documented, but doesn't exist in your code. Sorry, the file and line number could not be determined." + end end end From 463d4e0a1f0067fca8176ff44acb9c8f503fb58b Mon Sep 17 00:00:00 2001 From: Ian Kronquist Date: Mon, 29 Jun 2015 13:37:30 -0700 Subject: [PATCH 2/4] (PDOC-37) Attach params to Puppet 4.x code object Traverse the ruby AST to get method arguments and attach them to the code object. * Fixes spurious warnings issued by yard. * Obviates the need for ugly regex code in the templates to retrieve parameters from the source code. --- lib/puppet/face/strings.rb | 1 + .../handlers/puppet_4x_function_handler.rb | 27 ++++++++++++++-- .../templates/default/definedtype/setup.rb | 3 +- .../yard/templates/default/hostclass/setup.rb | 4 +++ .../default/puppetnamespace/setup.rb | 2 +- .../yard/templates/default/template_helper.rb | 31 ++++++++++++++----- 6 files changed, 56 insertions(+), 12 deletions(-) diff --git a/lib/puppet/face/strings.rb b/lib/puppet/face/strings.rb index e0221a5..48d6300 100644 --- a/lib/puppet/face/strings.rb +++ b/lib/puppet/face/strings.rb @@ -50,6 +50,7 @@ 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) + class YARD::Logger; def progress(*args); end; end yardoc_actions.generate_documentation(*yard_args) end diff --git a/lib/puppet_x/puppetlabs/strings/yard/handlers/puppet_4x_function_handler.rb b/lib/puppet_x/puppetlabs/strings/yard/handlers/puppet_4x_function_handler.rb index 29b7b22..27d6ebf 100644 --- a/lib/puppet_x/puppetlabs/strings/yard/handlers/puppet_4x_function_handler.rb +++ b/lib/puppet_x/puppetlabs/strings/yard/handlers/puppet_4x_function_handler.rb @@ -26,7 +26,30 @@ class Puppet4xFunctionHandler < YARD::Handlers::Ruby::Base process do name = process_parameters - obj = MethodObject.new(function_namespace, name) + method_arguments = [] + + # To attach the method parameters to the new code object, traverse the + # ruby AST until a node is found which defines a list of parameters. + # Then, traverse the children of the parameters, storing each identifier + # in the list of method arguments. + obj = MethodObject.new(function_namespace, name) do |o| + statement.traverse do |node| + if node.type == :params + node.traverse do |params| + if params.type == :ident + # FIXME: Replace nil with parameter types + method_arguments << [params[0], nil] + end + end + + # Now that the parameters have been found, break out of the traversal + break + end + end + end + + obj.parameters = method_arguments + obj['puppet_4x_function'] = true register obj @@ -78,7 +101,7 @@ class Puppet4xFunctionHandler < YARD::Handlers::Ruby::Base # # @return [(String, Hash{String => String})] def process_parameters - # Passing `false` to prameters excludes the block param from the returned + # Passing `false` to parameters excludes the block param from the returned # list. name, _ = statement.parameters(false).compact diff --git a/lib/puppet_x/puppetlabs/strings/yard/templates/default/definedtype/setup.rb b/lib/puppet_x/puppetlabs/strings/yard/templates/default/definedtype/setup.rb index 31d54fd..b11f1fa 100644 --- a/lib/puppet_x/puppetlabs/strings/yard/templates/default/definedtype/setup.rb +++ b/lib/puppet_x/puppetlabs/strings/yard/templates/default/definedtype/setup.rb @@ -18,7 +18,8 @@ def parameter_details @param_details = [] - @param_details = @template_helper.extract_param_details(params, param_tags, object.files, true) + @param_details = @template_helper.extract_param_details(params, param_tags, true) + @template_helper.check_parameters_match_docs object erb(:parameter_details) end diff --git a/lib/puppet_x/puppetlabs/strings/yard/templates/default/hostclass/setup.rb b/lib/puppet_x/puppetlabs/strings/yard/templates/default/hostclass/setup.rb index 081d237..f640d5b 100644 --- a/lib/puppet_x/puppetlabs/strings/yard/templates/default/hostclass/setup.rb +++ b/lib/puppet_x/puppetlabs/strings/yard/templates/default/hostclass/setup.rb @@ -3,6 +3,9 @@ include T('default/definedtype') def init super sections.push :subclasses + + @template_helper = TemplateHelper.new + @template_helper.check_parameters_match_docs object end def subclasses @@ -18,3 +21,4 @@ def subclasses return if @subclasses.nil? || @subclasses.empty? erb(:subclasses) end + diff --git a/lib/puppet_x/puppetlabs/strings/yard/templates/default/puppetnamespace/setup.rb b/lib/puppet_x/puppetlabs/strings/yard/templates/default/puppetnamespace/setup.rb index 5238f64..054cee2 100644 --- a/lib/puppet_x/puppetlabs/strings/yard/templates/default/puppetnamespace/setup.rb +++ b/lib/puppet_x/puppetlabs/strings/yard/templates/default/puppetnamespace/setup.rb @@ -75,7 +75,7 @@ def method_details_list # Convert the matched string into an array of strings params = parameters.nil? ? nil : parameters[1].split(/\s*,\s*/) - param_details = @template_helper.extract_param_details(params, param_tags, object.files) unless params.nil? + param_details = @template_helper.extract_param_details(params, param_tags) unless params.nil? else param_details = @template_helper.comment_only_param_details(param_tags) end diff --git a/lib/puppet_x/puppetlabs/strings/yard/templates/default/template_helper.rb b/lib/puppet_x/puppetlabs/strings/yard/templates/default/template_helper.rb index 030fd8f..64b236c 100644 --- a/lib/puppet_x/puppetlabs/strings/yard/templates/default/template_helper.rb +++ b/lib/puppet_x/puppetlabs/strings/yard/templates/default/template_helper.rb @@ -36,7 +36,7 @@ class TemplateHelper # :desc => [String] The description provided in the comment # :types => [Array] The parameter type(s) specified in the comment # :exists => [Boolean] True only if the parameter exists in the documented logic and not just in a comment} - def extract_param_details(parameters, tags_hash, locations, fq_name = false) + def extract_param_details(parameters, tags_hash, fq_name = false) parameter_info = [] # Extract the information for parameters that exist @@ -73,13 +73,6 @@ class TemplateHelper end if !param_exists parameter_info.push({:name => tag.name, :desc => tag.text, :types => tag.types, :exists? => false}) - if locations.length >= 1 and locations[0].length == 2 - file_name = locations[0][0] - line_number = locations[0][1] - $stderr.puts "[warn]: The parameter #{tag.name} is documented, but doesn't exist in your code, in file #{file_name} near line #{line_number}" - else - $stderr.puts "[warn]: The parameter #{tag.name} is documented, but doesn't exist in your code. Sorry, the file and line number could not be determined." - end end end @@ -111,4 +104,26 @@ class TemplateHelper parameter_info end + + # Check that the actual function parameters match what is stated in the docs. + # If there is a mismatch, print a warning to stderr. + # This is necessary for puppet classes and defined types. This type of + # warning will be issued for ruby code by the ruby docstring parser. + # @param object the code object to examine for parameters names + def check_parameters_match_docs(object) + param_tags = object.tags.find_all{ |tag| tag.tag_name == "param"} + names = object.parameters.map {|l| l.first.gsub(/\W/, '') } + locations = object.files + param_tags.each do |tag| + if not names.include?(tag.name) + if locations.length >= 1 and locations[0].length == 2 + file_name = locations[0][0] + line_number = locations[0][1] + $stderr.puts "[warn]: The parameter #{tag.name} is documented, but doesn't exist in your code, in file #{file_name} near line #{line_number}" + else + $stderr.puts "[warn]: The parameter #{tag.name} is documented, but doesn't exist in your code. Sorry, the file and line number could not be determined." + end + end + end + end end From d37e071ab7b3de63053675520ed25763d37582b7 Mon Sep 17 00:00:00 2001 From: Ian Kronquist Date: Wed, 1 Jul 2015 18:47:54 -0700 Subject: [PATCH 3/4] (PDOC-37) Test errors are output properly Also comment out a statement which we frequently use for debugging which snuck in by accident. --- lib/puppet/face/strings.rb | 6 +- .../yard/puppet_4x_function_handler_spec.rb | 23 +++++--- .../strings/yard/template_helper_spec.rb | 59 +++++++++++++++++++ 3 files changed, 80 insertions(+), 8 deletions(-) create mode 100644 spec/unit/puppet_x/puppetlabs/strings/yard/template_helper_spec.rb diff --git a/lib/puppet/face/strings.rb b/lib/puppet/face/strings.rb index 48d6300..26953bb 100644 --- a/lib/puppet/face/strings.rb +++ b/lib/puppet/face/strings.rb @@ -50,7 +50,11 @@ 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) - class YARD::Logger; def progress(*args); end; end + + # This line monkeypatches yard's progress indicator so it doesn't write + # all over the terminal. This should definitely not be in real code, but + # it's very handy for debugging with pry + #class YARD::Logger; def progress(*args); end; end yardoc_actions.generate_documentation(*yard_args) end diff --git a/spec/unit/puppet_x/puppetlabs/strings/yard/puppet_4x_function_handler_spec.rb b/spec/unit/puppet_x/puppetlabs/strings/yard/puppet_4x_function_handler_spec.rb index 14720e3..c59821c 100644 --- a/spec/unit/puppet_x/puppetlabs/strings/yard/puppet_4x_function_handler_spec.rb +++ b/spec/unit/puppet_x/puppetlabs/strings/yard/puppet_4x_function_handler_spec.rb @@ -54,12 +54,21 @@ describe PuppetX::PuppetLabs::Strings::YARD::Handlers::Puppet4xFunctionHandler d expect(the_namespace).to document_a(:type => :puppetnamespace) end - it "should not add anything to the Registry if incorrect ruby code is present" do - parse <<-RUBY - # The summary - This is not ruby code - RUBY - - expect(YARD::Registry.all).to be_empty + it "should issue a warning if the parameter names do not match the docstring" do + expected_output_not_a_param = "[warn]: @param tag has unknown parameter" + + " name: not_a_param \n in file `(stdin)' near line 3" + expected_output_also_not_a_param = "[warn]: @param tag has unknown " + + "parameter name: also_not_a_param \n in file `(stdin)' near line 3" + expect { + parse <<-RUBY + # @param not_a_param [Integer] the first number to be compared + # @param also_not_a_param [Integer] the second number to be compared + Puppet::Functions.create_function(:max) do + def max(num_a, num_b) + num_a >= num_b ? num_a : num_b + end + end + RUBY + }.to output("#{expected_output_not_a_param}\n#{expected_output_also_not_a_param}\n").to_stdout_from_any_process end end diff --git a/spec/unit/puppet_x/puppetlabs/strings/yard/template_helper_spec.rb b/spec/unit/puppet_x/puppetlabs/strings/yard/template_helper_spec.rb new file mode 100644 index 0000000..1f3a4b4 --- /dev/null +++ b/spec/unit/puppet_x/puppetlabs/strings/yard/template_helper_spec.rb @@ -0,0 +1,59 @@ +require 'spec_helper' +require 'puppet_x/puppetlabs/strings/yard/templates/default/template_helper' +require 'puppet_x/puppetlabs/strings/yard/code_objects/puppet_namespace_object' +require 'strings_spec/parsing' + +describe TemplateHelper do + it "should not print any warning if the tags and parameters match" do + th = TemplateHelper.new + + # Case 0: If the documented tags do include the parameter, + # nothing is printed + tag0 = YARD::Tags::Tag.new(:param, 'a_parameter') + tag0.name = 'a_parameter' + obj0 = PuppetX::PuppetLabs::Strings::YARD::CodeObjects::PuppetNamespaceObject.new(:root, 'Puppet3xFunctions') + obj0.add_tag tag0 + obj0.parameters = [['a_parameter']] + expect { th.check_parameters_match_docs obj0 }.to output("").to_stderr_from_any_process + + # The docstring is still alive between tests. Clear the tags registered with + # it so the tags won't persist between tests. + obj0.docstring.instance_variable_set("@tags", []) + end + + it "should print the warning with no location data if the tags and " + + "parameters differ and the location data is not properly formed" do + th = TemplateHelper.new + # Case 1: If the parameter and tag differ and the location is not properly + # formed, print out the warning with no location data + tag1 = YARD::Tags::Tag.new(:param, 'aa_parameter') + tag1.name = 'aa_parameter' + obj1 = PuppetX::PuppetLabs::Strings::YARD::CodeObjects::PuppetNamespaceObject.new(:root, 'Puppet3xFunctions') + obj1.add_tag tag1 + obj1.parameters = [['b_parameter']] + expect { th.check_parameters_match_docs obj1 }.to output("[warn]: The parameter aa_parameter is documented, but doesn't exist in your code. Sorry, the file and line number could not be determined.\n").to_stderr_from_any_process + + # The docstring is still alive between tests. Clear the tags registered with + # it so the tags won't persist between tests. + obj1.docstring.instance_variable_set("@tags", []) + end + + it "should print the warning with location data if the tags and parameters " + + "differ and the location data is properly formed" do + th = TemplateHelper.new + # Case 2: If the parameter and tag differ and the location is properly + # formed, print out the warning with no location data + tag2 = YARD::Tags::Tag.new(:param, 'aaa_parameter') + tag2.name = 'aaa_parameter' + obj2 = PuppetX::PuppetLabs::Strings::YARD::CodeObjects::PuppetNamespaceObject.new(:root, 'Puppet3xFunctions') + obj2.files = [['some_file.pp', 42]] + obj2.add_tag tag2 + obj2.parameters = [['b_parameter']] + expect { th.check_parameters_match_docs obj2 }.to output("[warn]: The parameter aaa_parameter is documented, but doesn't exist in your code, in file some_file.pp near line 42\n").to_stderr_from_any_process + + # The docstring is still alive between tests. Clear the tags registered with + # it so the tags won't persist between tests. + obj2.docstring.instance_variable_set("@tags", []) + end + +end From 168c1c4b532555365e8dd2a318a3fd463dcd8d85 Mon Sep 17 00:00:00 2001 From: Ian Kronquist Date: Mon, 6 Jul 2015 16:40:30 -0700 Subject: [PATCH 4/4] (PDOC-37) Override Yard logger on a per test basis Override Yard logger on a per test basis, not globally. Replace one hack with another slightly less disgusting one. --- spec/unit/puppet/face_spec.rb | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/spec/unit/puppet/face_spec.rb b/spec/unit/puppet/face_spec.rb index 03aad0d..5119c41 100644 --- a/spec/unit/puppet/face_spec.rb +++ b/spec/unit/puppet/face_spec.rb @@ -31,9 +31,25 @@ describe Puppet::Face do end describe "when generating HTML for documentation" do + + # HACK: In these tests we would like to suppress all output from the yard + # logger so we don't clutter up stdout. + # However, we do want the yard logger for other tests so we can + # assert that the right things are logged. To accomplish this, for + # this block of tests we monkeypatch the yard logger to be a generic + # stringio instance which does nothing and then we restore the + # original afterwards. + before(:all) do + @tmp = YARD::Logger.instance.io + YARD::Logger.instance.io = StringIO.new + end + + after(:all) do + YARD::Logger.instance.io = @tmp + end + it "should properly generate HTML for manifest comments" do - YARD::Logger.instance.io = StringIO.new using_module('test') do |tmp| Dir.chdir('test')