From f0e02240b7b75767a322bce4a3c90326dc89f85a Mon Sep 17 00:00:00 2001 From: Alexander Fisher Date: Fri, 2 Oct 2020 12:02:08 +0100 Subject: [PATCH] Warn if parameters that don't exist are documented Fixes #18 --- .../plugins/check_parameter_documentation.rb | 88 +++++++++------ .../check_parameter_documentation_spec.rb | 104 ++++++++++++++++-- 2 files changed, 152 insertions(+), 40 deletions(-) diff --git a/lib/puppet-lint/plugins/check_parameter_documentation.rb b/lib/puppet-lint/plugins/check_parameter_documentation.rb index 36c1aa2..eed275f 100644 --- a/lib/puppet-lint/plugins/check_parameter_documentation.rb +++ b/lib/puppet-lint/plugins/check_parameter_documentation.rb @@ -1,8 +1,6 @@ PuppetLint.new_check(:parameter_documentation) do def check class_indexes.concat(defined_type_indexes).each do |idx| - next if idx[:param_tokens].nil? - doc_params = {} doc_params_duplicates = Hash.new { |hash, key| hash[key] = [doc_params[key]] } is_private = false @@ -25,51 +23,75 @@ def check end end - next if is_private - - params = [] - e = idx[:param_tokens].each - begin - while (ptok = e.next) - if ptok.type == :VARIABLE - params << ptok - nesting = 0 - # skip to the next parameter to avoid finding default values of variables - while true - ptok = e.next - case ptok.type - when :LPAREN - nesting += 1 - when :RPAREN - nesting -= 1 - when :COMMA - break unless nesting > 0 - end - end - end - end - rescue StopIteration; end + params = extract_params(idx) # warn about duplicates doc_params_duplicates.each do |parameter, tokens| tokens.each do |token| notify :warning, { - :message => "Duplicate parameter documentation for #{parameter}", + :message => "Duplicate #{type_str(idx)} parameter documentation for #{parameter}", :line => token.line, :column => token.column + token.value.match(/\A\s*(@param\s*)?/)[0].length + 1 # `+ 1` is to account for length of the `#` COMMENT token. } end end - params.each do |p| - next if doc_params.has_key? p.value - idx_type = idx[:type] == :CLASS ? "class" : "defined type" + # warn about documentation for parameters that don't exist + doc_params.each do |parameter, token| + next if params.find { |p| p.value == parameter } + notify :warning, { - :message => "missing documentation for #{idx_type} parameter #{idx[:name_token].value}::#{p.value}", - :line => p.line, - :column => p.column, + :message => "No matching #{type_str(idx)} parameter for documentation of #{parameter}", + :line => token.line, + :column => token.column + token.value.match(/\A\s*(@param\s*)?/)[0].length + 1 } end + + unless is_private + params.each do |p| + next if doc_params.has_key? p.value + notify :warning, { + :message => "missing documentation for #{type_str(idx)} parameter #{idx[:name_token].value}::#{p.value}", + :line => p.line, + :column => p.column, + } + end + end end end + + private + + def type_str(idx) + idx[:type] == :CLASS ? "class" : "defined type" + end + + def extract_params(idx) + params = [] + return params if idx[:param_tokens].nil? + + e = idx[:param_tokens].each + begin + while (ptok = e.next) + if ptok.type == :VARIABLE + params << ptok + nesting = 0 + # skip to the next parameter to avoid finding default values of variables + while true + ptok = e.next + case ptok.type + when :LPAREN + nesting += 1 + when :RPAREN + nesting -= 1 + when :COMMA + break unless nesting > 0 + end + end + end + end + rescue StopIteration; end + + params + end end diff --git a/spec/puppet-lint/plugins/check_parameter_documentation_spec.rb b/spec/puppet-lint/plugins/check_parameter_documentation_spec.rb index 732ef1a..ea49486 100644 --- a/spec/puppet-lint/plugins/check_parameter_documentation_spec.rb +++ b/spec/puppet-lint/plugins/check_parameter_documentation_spec.rb @@ -564,11 +564,11 @@ class example($foo, $bar) { } end it 'should create a warning on line 3' do - expect(problems).to contain_warning('Duplicate parameter documentation for bar').on_line(3).in_column(10) + expect(problems).to contain_warning('Duplicate class parameter documentation for bar').on_line(3).in_column(10) end it 'should create a warning on line 6' do - expect(problems).to contain_warning('Duplicate parameter documentation for bar').on_line(6).in_column(10) + expect(problems).to contain_warning('Duplicate class parameter documentation for bar').on_line(6).in_column(10) end end @@ -594,15 +594,41 @@ class example($foo, $bar) { } end it 'should create a warning on line 3' do - expect(problems).to contain_warning('Duplicate parameter documentation for bar').on_line(3).in_column(10) + expect(problems).to contain_warning('Duplicate class parameter documentation for bar').on_line(3).in_column(10) end it 'should create a warning on line 6' do - expect(problems).to contain_warning('Duplicate parameter documentation for bar').on_line(6).in_column(10) + expect(problems).to contain_warning('Duplicate class parameter documentation for bar').on_line(6).in_column(10) end it 'should create a warning on line 8' do - expect(problems).to contain_warning('Duplicate parameter documentation for bar').on_line(8).in_column(10) + expect(problems).to contain_warning('Duplicate class parameter documentation for bar').on_line(8).in_column(10) + end + end + + context 'private class with parameters documented twice (@param)' do + let(:code) do + <<-EOS.gsub(/^\s+/, '') + # @summary Example class + # + # @param bar docs + # @param bar Duplicate/conflicting docs + # + # @api private + class example($bar) { } + EOS + end + + it 'should detect two problems' do + expect(problems).to have(2).problem + end + + it 'should create a warning on line 3' do + expect(problems).to contain_warning('Duplicate class parameter documentation for bar').on_line(3).in_column(10) + end + + it 'should create a warning on line 4' do + expect(problems).to contain_warning('Duplicate class parameter documentation for bar').on_line(4).in_column(10) end end @@ -625,11 +651,75 @@ class example($foo, $bar) { } end it 'should create a warning on line 3' do - expect(problems).to contain_warning('Duplicate parameter documentation for bar').on_line(3).in_column(10) + expect(problems).to contain_warning('Duplicate class parameter documentation for bar').on_line(3).in_column(10) end it 'should create a warning on line 6' do - expect(problems).to contain_warning('Duplicate parameter documentation for bar').on_line(6).in_column(3) + expect(problems).to contain_warning('Duplicate class parameter documentation for bar').on_line(6).in_column(3) + end + end + + context 'class with documentation for parameters that don\'t exist' do + let(:code) do + <<-EOS.gsub(/^\s+/, '') + # @summary Example class + # + # @param foo + class example { } + EOS + end + + it 'should detect a single problem' do + expect(problems).to have(1).problem + end + + it 'should create a warning on line 3' do + expect(problems).to contain_warning('No matching class parameter for documentation of foo').on_line(3).in_column(10) + end + end + + context 'private class with documentation for parameters that don\'t exist' do + let(:code) do + <<-EOS.gsub(/^\s+/, '') + # @summary Example class + # + # @param foo + # Example docs + # + # @api private + class example { } + EOS + end + + it 'should detect a single problem' do + expect(problems).to have(1).problem + end + + it 'should create a warning on line 3' do + expect(problems).to contain_warning('No matching class parameter for documentation of foo').on_line(3).in_column(10) + end + end + + context 'define with documentation for parameters that don\'t exist' do + let(:code) do + <<-EOS.gsub(/^\s+/, '') + # @summary Example define + # + # @param bar Docs for bar + # @param foo + # Docs for foo + # + # @api private + define example::example(String[1] $bar) { } + EOS + end + + it 'should detect a single problem' do + expect(problems).to have(1).problem + end + + it 'should create a warning on line 4' do + expect(problems).to contain_warning('No matching defined type parameter for documentation of foo').on_line(4).in_column(10) end end end