Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for rubocop-rspec 0.20.1 #70

Merged
merged 2 commits into from
Dec 8, 2017
Merged

Fix for rubocop-rspec 0.20.1 #70

merged 2 commits into from
Dec 8, 2017

Conversation

mrkn
Copy link
Contributor

@mrkn mrkn commented Dec 7, 2017

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

@mrkn
Copy link
Contributor Author

mrkn commented Dec 7, 2017

But I strongly recommend removing rubocop from the gemspec.

@zverok
Copy link
Collaborator

zverok commented Dec 7, 2017

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 Update the dependent daru version #68
RSpec/ContextWording:
  Enabled: false
  1. Add GitHub issue to fix it properly.

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

@mrkn
Copy link
Contributor Author

mrkn commented Dec 7, 2017

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
Copy link
Collaborator

zverok commented Dec 7, 2017

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
Copy link
Contributor Author

mrkn commented Dec 8, 2017

Who can merge this pull-request?
I cannot.

@athityakumar
Copy link
Member

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

@athityakumar athityakumar merged commit 8c52744 into SciRuby:master Dec 8, 2017
@mrkn
Copy link
Contributor Author

mrkn commented Dec 8, 2017

@athityakumar @zverok thanks!!

@mrkn mrkn deleted the rubocop branch December 8, 2017 05:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants