Make compatible with latest RuboCop

[craiglittle]

This brings it up-to-date for v0.36.0.

Changelog: https://github.com/bbatsov/rubocop/blob/master/CHANGELOG.md#0360-14012016

[colszowka]

I like the approach, but this changes the logic, you're missing an else SimpleCov::ResultMerger.merged_result case here to keep it in line with the previous version, no?

[craiglittle]

Whoops, I think the right move is to get rid of the now-redundant if defined? @result. On the way.

[colszowka]

Reading this method makes me dizzy, maybe we get to untangle this a bit:

def result
  # Ensure the variable is defined to avoid ruby warnings
  @result = nil unless defined?(@result)

  # Collect our coverage result
  if running and not result?
    @result = SimpleCov::Result.new add_not_loaded_files(Coverage.result)
  end

  # If we're using merging of results, store the current result
  # first, then merge the results and return those
  if use_merging
    if result?
      SimpleCov::ResultMerger.store_result @result
    end
    SimpleCov::ResultMerger.merged_result
  else
    @result
  end
ensure
  self.running = false
end

[colszowka]

I know this is not applied to all of the code base and this especially is "just" our test sample code, but personally I prefer the actual if/else form because it's easier to parse the control flow of the method

[craiglittle]

Sure. Would you like me to disable that cop completely or special case this file?

[colszowka]

Which one is it and what is it complaining about? Can't see anything in the build log :(

[craiglittle]

That would be the Style::GuardClause cop, which says:

Use a guard clause instead of wrapping the code inside a conditional expression.

[craiglittle]

I've added the refactor of the result method with a few tweaks to continue to make RuboCop happy. Now, all we need to resolve is what to do with the guard clause.

[craiglittle]

Also, I think it'd be prudent to consider locking rubocop to a minor version so it doesn't randomly break the build whenever a new version is released. Then, it could be updated when it's convenient instead of blocking other PRs.

Would you be open to me making that change?

[craiglittle]

@colszowka When you get a chance, I'm waiting on feedback on two points in order to proceed:

• What should we do about the Style/GuardClause cop?
• Should we lock down rubocop to avoid unexpected build failures in the future?

[colszowka]

Thanks for keeping up on this thing!

• As I mentioned before, nowadays I personally prefer explicit conditionals over guard clauses because imho they're easier to read. But that also requires you to make your conditionals clean. I think in this case that is true, but its my personal opinion. Maybe you could create a follow-up issue where we bring in the rest of the contributors team to figure this out, and disable it for now to resolve the PR?
• yes please, I was thinking about this myself. Please lock it at patch level since we're currently at 0.36 if I remember correctly, so ~>0.36 wouldnt fix the problem :)

Thanks again

On Fri, Jan 22, 2016 at 2:14 PM -0800, "Craig Little" notifications@github.com wrote:

@colszowka When you get a chance, I'm waiting on feedback on two points in order to proceed:

What should we do about the Style/GuardClause cop?
Should we lock down rubocop to avoid unexpected build failures in the future?

—
Reply to this email directly or view it on GitHub.

[craiglittle]

As I mentioned before, nowadays I personally prefer explicit conditionals over guard clauses because imho they're easier to read. But that also requires you to make your conditionals clean. I think in this case that is true, but its my personal opinion. Maybe you could create a follow-up issue where we bring in the rest of the contributors team to figure this out, and disable it for now to resolve the PR?

Done and done!

yes please, I was thinking about this myself. Please lock it at patch level since we're currently at 0.36 if I remember correctly, so ~>0.36 wouldnt fix the problem :)

Locked to the v0.36.x series.

And the build's 💚!

[colszowka]

Perfect, thanks @craiglittle