-
Notifications
You must be signed in to change notification settings - Fork 21
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
Support Puppet >= 4.10 #50
Conversation
Rubocop seems to become more picky than in the past. I see 40 offenses within the existing code and this change is not even touching ruby code. |
Remove dependency on 'pe' as it will fail in any metadata-json-lint tests.
Bundler/DuplicatedGem: | ||
Exclude: | ||
- "Gemfile" | ||
|
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.
see rubocop/rubocop#3752 if interessted in more details
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.
Wow, what a mess. Thanks for figuring this out.
Metrics/BlockLength: | ||
Exclude: | ||
- "spec/**/*.rb" | ||
|
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.
see rubocop/rubocop#3772 if interessted in more details
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.
Why disable entirely on spec tests? Is the idea that we just use it for actual ruby code in lib/
?
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.
This cop get's angry about code blocks with more than 25 lines. It would need a complete refactor to satisfy him. The longest block is 1487 lines at the moment.
I think this cop is good for real ruby code but not needed on the spec tests itself.
# This cop isn't compatible with Ruby < 2.0 | ||
Style/SymbolArray: | ||
Exclude: | ||
- "Rakefile" |
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.
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.
Why ignore the Rakefile? I thought that since it is ruby, it should be tested. I'm not sure what I was supposed to get from looking at that Travis link.
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.
This cop enforces to use a syntax that is not available on Ruby < 2.0.
You can see the actual error message in one of the failed tests [1].
[1] https://travis-ci.org/ghoneycutt/puppet-module-nscd/jobs/220867193#L213-L216
Yes, rubocop is not very stable. I'm all for standardized code but not at the cost of just continually churning it around. I'm happy to add a bunch of ignore/exclude lines :) |
@@ -740,7 +740,7 @@ | |||
end | |||
|
|||
context 'as an array' do | |||
let(:params) { { :package_name => %w(nscd foo) } } | |||
let(:params) { { :package_name => %w[nscd foo] } } |
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.
this seems like rubocop nonsense
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.
If you don't like that, I can turn of the responding cop too.
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.
Please do turn off the cop that wants to change all the %w()
to %w[]
. This way we reduce code churn and make other PR's easier to merge. Funny thing is we went to %w()
because of rubocop :>
Other than that, everything looks good! Thanks!
@ghoneycutt please keep in mind that I only have disabled specific cops for specific files. This is the minimally invasive configuration change I could think of. |
@@ -740,7 +740,7 @@ | |||
end | |||
|
|||
context 'as an array' do | |||
let(:params) { { :package_name => %w(nscd foo) } } | |||
let(:params) { { :package_name => %w[nscd foo] } } |
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.
Please do turn off the cop that wants to change all the %w()
to %w[]
. This way we reduce code churn and make other PR's easier to merge. Funny thing is we went to %w()
because of rubocop :>
Other than that, everything looks good! Thanks!
Released in v1.10.0 |
No description provided.