Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warn if a parameter is documented more than once and remove support for multi-line (/* */) comments #19

Merged
merged 2 commits into from Oct 2, 2020

Conversation

alexjfisher
Copy link
Member

Fixes #16

@@ -11,7 +11,7 @@ def check
if dtok.value =~ /\A\s*\[\*([a-zA-Z0-9_]+)\*\]/ or
dtok.value =~ /\A\s*\$([a-zA-Z0-9_]+):: +/ or
dtok.value =~ /\A\s*@param (?:\[.+\] )?([a-zA-Z0-9_]+)(?: +|$)/
doc_params << $1
doc_params.unshift( { value: $1, token: dtok }) # Push new params onto start of array
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about Hash doc_params. If the param already exists, add it to Array duplicates.

You may want to consider also adding a warning on the line with the original. This can make it easy to find the pair of duplicates and merge them. However, not strictly needed and it's probably easy enough if you don't.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering why you wanted a hash, but I guess that'll be needed for fixing some of the other issues I opened.

notify :warning, {
:message => "Duplicate parameter documentation for #{p[:value]}",
:line => p[:token].line,
:column => p[:token].column
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The column will have a mismatch since there is a regex. You may want to consider p[:token].column + p[:token].value.match(/\A\s*/)[0].length (which is OK because all regexes around line 11 start with whitespace.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tokens with @param have two sets of whitespace. eg p[:token].value == ' @param foo'

Copy link
Member

@ghoneycutt ghoneycutt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests are great

@alexjfisher alexjfisher requested a review from ekohl October 1, 2020 18:39
alexjfisher added a commit to alexjfisher/puppet-puppet that referenced this pull request Oct 1, 2020
alexjfisher added a commit to alexjfisher/puppet-pulp that referenced this pull request Oct 1, 2020
@alexjfisher alexjfisher changed the title Warn if a parameter is documented more than once Warn if a parameter is documented more than once and remove support for multi-line (/* */) comments Oct 1, 2020
@alexjfisher alexjfisher requested a review from ekohl October 1, 2020 21:43
@alexjfisher
Copy link
Member Author

Tests are great

Thanks!

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how rspec treats duplicate tests, but at least I got confused when I read the code. Other than that 👍

ekohl pushed a commit to theforeman/puppet-puppet that referenced this pull request Oct 2, 2020
ekohl pushed a commit to theforeman/puppet-pulp that referenced this pull request Oct 2, 2020
@alexjfisher alexjfisher marked this pull request as draft October 2, 2020 09:30
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Untested, but I think this is slightly cleaner. I think it also means you get all duplicate parameters grouped in the output rather than when they occur.

lib/puppet-lint/plugins/check_parameter_documentation.rb Outdated Show resolved Hide resolved
lib/puppet-lint/plugins/check_parameter_documentation.rb Outdated Show resolved Hide resolved
lib/puppet-lint/plugins/check_parameter_documentation.rb Outdated Show resolved Hide resolved
Slash comments `//` don't even work and aren't in the Puppet 4+ language
spec.  Core puppet-lint already warns against multi-line comments and
they were removed from the Puppet 6 docs.  We also have zero tests using
them in this plugin.
@alexjfisher alexjfisher marked this pull request as ready for review October 2, 2020 10:02
@alexjfisher alexjfisher merged commit 8628fb5 into voxpupuli:master Oct 2, 2020
@alexjfisher alexjfisher deleted the issue_16 branch October 2, 2020 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplicate parameter documentation should flag an error
3 participants