search

SciRuby / daru-io

daru-io is a plugin gem to the existing daru gem, which aims to add support to Importing DataFrames from / Exporting DataFrames to multiple formats.
http://www.rubydoc.info/github/athityakumar/daru-io/master/
MIT License
24 stars 9 forks source link

Fix for rubocop-rspec 0.20.1 #70

Closed mrkn closed 6 years ago

mrkn commented 6 years ago

Before #68, fixes against rubocop-rspec 0.20.1 needs to be merged.

mrkn commented 6 years ago

But I strongly recommend removing rubocop from the gemspec.

zverok commented 6 years ago

But I strongly recommend removing rubocop from the gemspec.

I strongly recommend NOT to.

As SciRuby basically lives on students/novice contributions, automatical style check is a blessing. The trick is, rubocop (and rubocop-rspec especially) is not a God's Voice and can be easily set up as we like it. So, in this case, you could just disable this new cop in .rubocop.yml and call it a day if it stays in the way of important work.

On the other hand, I believe that this suggestion (that context description should look like "context: when/with " not "context: does ") is pretty meaningful and good to guard.

On the third hand (yep feeling a bit like Shiva today) fixing it mechanically, like this PR does, brings no value. For ex.:

-  context 'writes convert_comma only on float values' do
+  context 'when writes convert_comma only on float values' do

The latter reads really weird. The "proper" fix for this cop is, in fact:

-  context 'writes convert_comma only on float values' do
+  describe ':convert_comma option' do

...but fixing things this way will take a lot of time.

So, what I suggest in fact is:

  1. Close this PR
  2. Add to #68 
    RSpec/ContextWording:
Enabled: false
  3. Add GitHub issue to fix it properly.

As an outtake: Rubocop brings value, but mechanical following the cops does not.

mrkn commented 6 years ago

Is disabling RSpec/ContextWording acceptable? If it is yes, I want to disable it in this PR but #68 because #68 isn't related to rubocop.

zverok commented 6 years ago

Is disabling RSpec/ContextWording acceptable?

Absolutely! Rule of thumb with new Rubocop cops, that emerged in the new version and suddenly offended by half of the codebase:

  1. If the cop has autocorrect feature AND it is not too large change in style — use autocorrect.
  2. If the cop has settings and can be set to our codebase standards (which are consistent, but different from cop defaults) — do it.
  3. If it is neither — disable it and postpone fixing (if it is reasonable at all).

RSpec/ContextWording is clearly case (3).

mrkn commented 6 years ago

Who can merge this pull-request? I cannot.

athityakumar commented 6 years ago

I was waiting for @zverok to do the deed, but I'll do it now. 😄

mrkn commented 6 years ago

@athityakumar @zverok thanks!!