rubocop / ruby-style-guide

A community-driven Ruby coding style guide
https://rubystyle.guide
16.46k stars 3.4k forks source link

It shouldn't be required to use if/unless on the same line as do_something if code coverage tools are being used #286

Closed abotalov closed 5 years ago

abotalov commented 10 years ago

Styleguide has a rule:

Favor modifier if/unless usage when you have a single-line body.

do_something if some_condition

But it seems that Ruby's Coverage module (and thus other coverage tools like Simplecov based on it) support only line coverage and thus I won't see if do_something was invoked during test suite.

So I propose to change rule to:

Favor modifier if/unless usage when you have a single-line body if you don't use code coverage tools.

# ok (if you use code coverage tools)
if some_condition
  do_something
end
bbatsov commented 10 years ago

Interesting point - I had never thought about this. However, it seems to me that we shouldn't sacrifice code clarity to compensate for tool deficiencies.

abotalov commented 10 years ago

@bbatsov Actually Coverage module is part of MRI. So it's not the tool - it's Ruby itself.

It seems to me that until Ruby implementations will provide a way to check if part of line of code has been executed Ruby style guide should take current situation into account.

arr-dev commented 10 years ago

I was just thinking about creating this ticket, but search saved me. I also think that this is something that should be recommended since coverage has no way of checking this.

jdickey commented 10 years ago

Agreed (with @abotalov and @soul-rebel). Coverage (and Simplecov, etc) are line-coverage tools passing themselves off as statement coverage, or C0. Nobody seems to have a C1-level (branch coverage) tool for Ruby, per Stack Overflow and further explained over at Code Climate. IMO, the team that delivers reliable C1 and C2 (execution-path) coverage for Ruby will be heroes well above the level of the (excellent) rails-core team.

Declaring that we "shouldn't sacrifice code clarity to compensate for tool deficiencies" is a comforting ideal, but in the real world, it's not going to fly very far. Yes, if you're doing BDD/TDD "right", you should never have less than 100% (C0-ish) coverage. The mere existence of CodeClimate as a viable business shows that many teams don't meet that ideal. It's a virtual certainty that multiple Gems on which we all depend, don't meet that ideal. Given that, a useful tool. practice or standard acts to compensate for the weaknesses of other commonly-used tools when it is not overly disruptive to do so.

Just my 0.84 cents (ruddy inflation!)

lee-dohm commented 10 years ago

I'd support the modification as long as it specifically states "code coverage tools that only support line coverage" with perhaps a link like what @jdickey mentions. I agree with @bbatsov that in general we shouldn't sacrifice code clarity because of tools, we should just fix the tools ... but I also feel that giving an exception in very specific circumstances isn't bad either.

bbatsov commented 10 years ago

I'm pretty sure the majority of Ruby users would simply discard the suggested change. Ruby has always placed strong emphasis on readability and make a compromise with readability for the sake of tooling is unlikely to resonate with them. Personally, I'll never drop the modifier form. I use simplecov as well, but don't really care whether the code metric is accurate or not. One can get excellent coverage with a pretty crappy test suite; I care about the quality of tests.

bbatsov commented 5 years ago

I'm closing this ticket due to no recent activity. Feel free to re-open it if you ever come back to it.