Fix all remaining rubocop violations #241
Conversation
Signed-off-by: Alexander Fisher <[email protected]>
Signed-off-by: Alexander Fisher <[email protected]>
Signed-off-by: Alexander Fisher <[email protected]>
Thanks to elomatreb on #ruby IRC Signed-off-by: Alexander Fisher <[email protected]>
Give block to `match` instead of using an `if` See https://ruby-doc.org/core-2.1.1/Regexp.html#method-i-match Signed-off-by: Alexander Fisher <[email protected]>
|
Thanks to @elomatreb on IRC #ruby for the advice on 3f6cef9 and 27af372 |
| @@ -36,7 +36,7 @@ def self.pearlist(hash) | |||
| pearhash[:provider] = :pear | |||
| pearhash | |||
| end | |||
| end.reject { |p| p.nil? } | |||
| end.reject(&:nil?) | |||
There was a problem hiding this comment.
I was wondering if this could actually be replaced with end.compact, but was too scared!
There was a problem hiding this comment.
Array#compact should work, the only difference to this manual solution I could find was that compact will only remove actual nil, whereas this will work on classes that define a custom #nil? (I see no reason why that would be something you want though).
|
LGTM but would be cool if somebody else could review that too. |
| @@ -30,9 +30,8 @@ module Puppet::Parser::Functions | |||
|
|
|||
| raise(Puppet::ParseError, 'to_hash_settings(): Requires hash to work with') unless hash.is_a?(Hash) | |||
|
|
|||
| return hash.reduce({}) do |acc, kv| | |||
| return hash.each_with_object({}) do |kv, acc| | |||
There was a problem hiding this comment.
We actually have a spec test for this function, and it still passes! Yay!
| @@ -20,19 +20,17 @@ def self.pecllist(hash) | |||
| begin | |||
| list = execute(command).split("\n").map do |set| | |||
| if hash[:justme] | |||
| if %r{^#{hash[:justme]}$}i.match(set) | |||
| if %r{^#{hash[:justme]}$}i =~ set | |||
| if peclhash = peclsplit(set) # rubocop:disable Lint/AssignmentInCondition | |||
There was a problem hiding this comment.
Can these conditionals be merged into one if (to avoid nesting)?
There was a problem hiding this comment.
Good spot. It certainly looks like it, doesn't it? Lets look at the original code before any rubocop and refactoring started... https://github.com/voxpupuli/puppet-php/blob/4.0.0-beta1/lib/puppet/provider/package/pear.rb#L29
So assuming 6bbdc78 was correct and rubocop --auto-correct --only Style/EmptyElse wasn't mistaken, then yes to merging the conditionals?
It's a real shame there are no spec tests covering this code...
There was a problem hiding this comment.
The two versions should be functionally equivalent, as a non-branching if returns nil in Ruby.
I guess the real question is if this:
if %r{^#{hash[:justme]}$}i =~ set
if peclhash = peclsplit(set) # rubocop:disable Lint/AssignmentInCondition
peclhash[:provider] = :peclcmd
peclhash
end
endis more readable than:
if %r{^#{hash[:justme]}$}i =~ set &&
peclhash = peclsplit(set) # rubocop:disable Lint/AssignmentInCondition
peclhash[:provider] = :peclcmd
peclhash
endbtw: Are you sure using a disable comment everywhere were you want to use assignment in a condition is the best way to do it? Rubocop also allows assignment in condition if it is wrapped in parentheses, like so:
if %r{^#{hash[:justme]}$}i =~ set &&
(peclhash = peclsplit(set))
peclhash[:provider] = :peclcmd
peclhash
endSigned-off-by: Alexander Fisher <[email protected]>
|
I've added another commit with the extra refactoring. Somebody might want to double check the diff of the two provider files from before I started touching them. |
No description provided.