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

Add an option to override ignore private apis #28

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vStone
Copy link
Contributor

@vStone vStone commented Feb 16, 2021

No description provided.

@@ -1,5 +1,8 @@
PuppetLint.new_check(:parameter_documentation) do
def check

ignore_private = PuppetLint.configuration.docs_ignore_private_api.nil? ? true : PuppetLint.configuration.docs_ignore_private_api
Copy link
Member

Choose a reason for hiding this comment

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

I looked into a cleaner way to add configuration but it looks like there isn't really a good way. Perhaps it can be simplified to

Suggested change
ignore_private = PuppetLint.configuration.docs_ignore_private_api.nil? ? true : PuppetLint.configuration.docs_ignore_private_api
ignore_private = PuppetLint.configuration.docs_ignore_private_api.nil? || PuppetLint.configuration.docs_ignore_private_api

Not sure if it's cleaner. The cleanest way would be to have an API to also add a default.

Perhaps a much better way is to avoid the negative name and name it docs_check_private_params. That can default to false. Since nil and false are equivalent in this case, it would solve the problem.

The only downside is that the unless condition becomes more complex, but overall I think it's a win. Let me know what you think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no opinion on the naming. adjustments have been made :)

@vStone vStone force-pushed the feature/ignore_private_api branch from 89a8f96 to 53e2f92 Compare February 16, 2021 13:18
@vStone vStone force-pushed the feature/ignore_private_api branch from 53e2f92 to cb3a376 Compare February 16, 2021 13:19
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.

Small nits, otherwise 👍

@@ -1,5 +1,7 @@
PuppetLint.new_check(:parameter_documentation) do
def check
check_private = PuppetLint.configuration.docs_check_private_params || false
Copy link
Member

Choose a reason for hiding this comment

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

nil and false evaluate to the same

Suggested change
check_private = PuppetLint.configuration.docs_check_private_params || false
check_private = PuppetLint.configuration.docs_check_private_params

@@ -50,7 +52,7 @@ def check
}
end

unless is_private
unless is_private and not check_private
Copy link
Member

Choose a reason for hiding this comment

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

Ruby uses && instead of and (same for || vs or)

Suggested change
unless is_private and not check_private
unless is_private && !check_private

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants