-
Notifications
You must be signed in to change notification settings - Fork 54
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 new cop ChefModernize/ConditionalUsingTest #666
Conversation
Avoid shelling out when we can just use Ruby which is going to be faster. Signed-off-by: Tim Smith <tsmith@chef.io>
not_if { ::File.exist?('/sbin/apt') } | ||
end | ||
RUBY | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could see this being a tiny bit confusing if the user uses test -e
and this suggests not using test -f
.
Also this is the nitpickiest of nitpicks, but someone could want the behavior of -f
that does more than test for existence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah -f is a bit different than File.exist?, but looking at how users were using it throughout supermarket cookbooks they were just looking for File.exist? functionality. Someone out there isn't going to like this and for that user they can disable it inline or entirely in a .rubocop.yml. For most people, I think it gives the appropriate direction.
Signed-off-by: Tim Smith <tsmith@chef.io> Co-authored-by: pete higgins <pete@peterhiggins.org>
Signed-off-by: Tim Smith <tsmith@chef.io> Co-authored-by: pete higgins <pete@peterhiggins.org>
Signed-off-by: Tim Smith <tsmith@chef.io> Co-authored-by: pete higgins <pete@peterhiggins.org>
Avoid shelling out when we can just use Ruby which is going to be faster.
Fixes #543
Signed-off-by: Tim Smith tsmith@chef.io