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

Make Style/SymbolArray cop to enable by default #4124

Closed
pocke opened this issue Mar 15, 2017 · 6 comments
Closed

Make Style/SymbolArray cop to enable by default #4124

pocke opened this issue Mar 15, 2017 · 6 comments

Comments

@pocke
Copy link
Collaborator

pocke commented Mar 15, 2017

Expected behavior

Style/SymbolArray:
  Enabled: true

Actual behavior

Style/SymbolArray:
  Enabled: false

I think the Style/SymbolArray cop should be enabled by default, but the cop is disabled by default.
https://github.com/bbatsov/rubocop/blob/master/config/disabled.yml#L117-L120
40cf684
When the cop was added, TargetRubyVersion config does not exist.
However currently TargetRubyVersion exists, so we can make the cop to enable by default.

However, the cop has some problems in order to make to enable by default.

Handle TargetRubyVersion

I guess the cop does not properly handle TargetRubyVersion config.
https://github.com/bbatsov/rubocop/blob/a0f3a8f1223ba8d5b45e1bfbd950ff6f81b275f7/lib/rubocop/cop/style/symbol_array.rb#L27-L38
I think the validate_config method is unnecessarily. We should use minimum_target_ruby_version instead of validate_config.

Bug

The cop has a bug.

For example:

.rubocop.yml

Style/SymbolArray:
  Enabled: true
  EnforcedStyle: brackets

test.rb

%i(a a-b)
$ rubocop --only SmybolArray -a
Inspecting 1 file
C

Offenses:

test.rb:1:1: C: [Corrected] Use [] for an array of symbols.
%i[a a-b]
^^^^^^^^^

1 file inspected, 1 offense detected, 1 offense corrected

$ cat test.rb
[:a, :a-b]

The meaning of %i(a a-b) and [:a, :a-b] is different, but RuboCop add an offense for this code.

Auto correct

The cop supports auto correction. However I think the feature is not enough.

For example

FILTER_METHODS = [
  :after_filter,
  :append_after_filter,
  :append_around_filter,
  :append_before_filter,
  :around_filter,
  :before_filter,
  :prepend_after_filter,
  :prepend_around_filter,
  :prepend_before_filter,
  :skip_after_filter,
  :skip_around_filter,
  :skip_before_filter,
  :skip_filter
].freeze
FILTER_METHODS = %i(after_filter append_after_filter append_around_filter append_before_filter around_filter before_filter prepend_after_filter prepend_around_filter prepend_before_filter skip_after_filter skip_around_filter skip_before_filter skip_filter).freeze

The first code is auto-corrected to second code. The second code is not readable. We will need to add lien breaks manually to this code.

Style/WordArray supports line brakes feature, so, we should support the feature for Style/SymbolArray as well.


I'll implement the above features, and I'll make to enable by default the cop when finished.

@bbatsov
Copy link
Collaborator

bbatsov commented Mar 16, 2017

I'll implement the above features, and I'll make to enable by default the cop when finished.

Fine by me. The reason this cop is not enabled by default is that it was developed when we still supported Ruby 1.9.

pocke added a commit to pocke/rubocop that referenced this issue Mar 19, 2017
pocke added a commit to pocke/rubocop that referenced this issue Mar 19, 2017
pocke added a commit to pocke/rubocop that referenced this issue Mar 19, 2017
…Array` cop

Bad code

```ruby
[
  :foo, :bar,
  :baz
]
```

Correction

```ruby
 # before
%i[foo bar baz]

 # after
%i[
  foo bar
  baz
]
```
pocke added a commit to pocke/rubocop that referenced this issue Mar 19, 2017
pocke added a commit to pocke/rubocop that referenced this issue Mar 19, 2017
pocke added a commit to pocke/rubocop that referenced this issue Mar 19, 2017
pocke added a commit to pocke/rubocop that referenced this issue Mar 19, 2017
bbatsov pushed a commit that referenced this issue Mar 19, 2017
Bad code

```ruby
[
  :foo, :bar,
  :baz
]
```

Correction

```ruby
 # before
%i[foo bar baz]

 # after
%i[
  foo bar
  baz
]
```
bbatsov pushed a commit that referenced this issue Mar 19, 2017
@LunaCodeGirl
Copy link

For the record, I disagree. This seems like a strange thing to force on people by default given there is no inherent advantage, and it adds cognitive overload for developers.

@vvo
Copy link

vvo commented Jan 10, 2020

@LunaCodeGirl I am late but agrees, as a newcomer Ruby is complex enough and I wish this was not enabled by default. Pure preference but the least amount of tokens I have to learn the better, I had to learn what was a Symbol and now rubocop introduces %i on top of it ahah :D

@LunaCodeGirl
Copy link

I just think its purely preferential and is doing nothing to add to code readability and if anything its decreasing it. This should not be enabled by default.

@pocke
Copy link
Collaborator Author

pocke commented Jan 11, 2020

Thanks for your feedback.
RuboCop's default rule is based on the Ruby Style Guide, so if you'd like to change the default, I recommend to open an issue to the style guide (opening an issue to this repository is also ok).
https://github.com/rubocop-hq/ruby-style-guide#i

By the way, personally I still think this cop should be enabled by default.
I agree with the cop does not improve readability.
But it improves writability by reducing typing : and , in my opinion. You can find good examples from this file. https://github.com/rubocop-hq/rubocop/blob/7e37a77d40e0cb45625bb72c7af051c6fad6932a/lib/rubocop/ast/node.rb

And, I think not all Cops improve readability directly. Readability is important, but RuboCop also focuses on consistency. We can enforce ourselves that we use %i[] or [] with this cop.

@MaG21
Copy link

MaG21 commented Oct 18, 2020

For anyone here wondering how to disable this madness, here's a snippet copy/paste ready.

in your .rubocop.yml:

Style/SymbolArray:
  Enabled: false

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants