(maint) rubocop cleanup & pdksync #111
Conversation
tphoney
force-pushed
the
rubocop_cleanup
branch
9 times, most recently
from
a55f671 to
4c6860a
Compare
|
|
| end | ||
|
|
||
| raise "Unable to validate property #{name} (type #{mof_type[:type]}) of #{embeddedinstance_name}: value #{value}" unless respond_to?(validation_method) | ||
| send(validation_method, name, value) |
There was a problem hiding this comment.
I'm trying to understand why this line is equivalent to the previous if/else statement. Is the intention here that if the raise statement is fired that execution should stop and the send would not be hit? If execution is allowed to continue after the raise fires and send is not skipped, then this rearragement would not be equivalent to the prior way the code was arranged.
There was a problem hiding this comment.
a raise is an exception, and should be treated as such. the send line, is the last statement in the function, and the actual implicit return
There was a problem hiding this comment.
A Rubocop disable here as well would be appreciated.
spec/lib/dsc_utils.rb
Outdated
| end | ||
| return unless host['roles'].include?('master') | ||
| step 'Sync DSC resource implementations from master to agents' | ||
| on(agents, puppet('agent -t --environment production'), acceptable_exit_codes: [0, 2]) |
There was a problem hiding this comment.
The intent of the code seems much clearer under the former arrangement. It took me a little while of staring at this before I was able to work out how these two sets of code were equivalent. If we revert this to the if block style will Rubocop complain?
There was a problem hiding this comment.
i would argue the opposite. what happens when the if statement fails, there is no clear return.
There was a problem hiding this comment.
Ruby's implicit returns make it difficult to tell based on either form of this code block where the return should come from. Even on this form, should the return come from line 91? There's another on call on line 83, is that one returning data also? Really, neither of them are returning anything, and the intent of this function is not to return at all, but to perform the steps like the one on 90.
And the return unless doesn't make it clearer what the return should be since it's not really returning anything but being used to halt execution. It just makes it less clear that the two lines below should always be treated as a single unit with the conditional above.
There was a problem hiding this comment.
As requested, I would like a Rubocop disable here if possible.
|
re run with the changes |
|
|
No description provided.