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

Add new Style/StringChars cop #9615

Merged
merged 1 commit into from Mar 18, 2021

Conversation

koic
Copy link
Member

@koic koic commented Mar 18, 2021

Summary

This PR adds new Style/StringChars cop. The cop checks for uses of String#split with empty string or regexp literal argument.

# bad
string.split(//)
string.split('')

# good
string.chars

Since Ruby 2.0, the behavior of String#chars and String#split(//) is the same.

Ruby 1.9 and lower

% ruby -ve "p 'foo'.split(//)"
ruby 1.9.3p551 (2014-11-13 revision 48407) [x86_64-darwin13.0.2]
["f", "o", "o"]
% ruby -ve "p 'foo'.chars"
ruby 1.9.3p551 (2014-11-13 revision 48407) [x86_64-darwin13.0.2]
#<Enumerator: "foo":chars>

Ruby 2.0 and higher

% ruby -ve "p 'foo'.split(//)"
ruby 2.0.0p648 (2015-12-16 revision 53162) [x86_64-darwin13.0.2]
["f", "o", "o"]
% ruby -ve "p 'foo'.chars"
ruby 2.0.0p648 (2015-12-16 revision 53162) [x86_64-darwin13.0.2]
["f", "o", "o"]

Writing String#chars would pretty express the intent of the code.

Additional Information

This cop is intended to use String#chars method that express readable code.
And it has a different purpose than Performance/RedundantSplitRegexpArgument cop.
https://docs.rubocop.org/rubocop-performance/cops_performance.html#performanceredundantsplitregexpargument

Therefore, it will be added as a new Style cop.


Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

@koic koic force-pushed the add_new_style_string_chars_cop branch from 8122e36 to c5fe028 Compare March 18, 2021 16:10
@@ -4493,6 +4493,11 @@ Style/StderrPuts:
Enabled: true
VersionAdded: '0.51'

Style/StringChars:
Description: 'checks for uses of `String#split` with empty string or regexp literal argument.'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you should mention that the cops is not safe and add some notes about the safety.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, probably it's quite unlikely that some other class would define a split method that takes exactly the same args. :-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the review. I marked the cop as unsafe and added a description to safety.
Initially, I didn't mark it as unsafe because I think it's less likely to be false positives 😅

@bbatsov
Copy link
Collaborator

bbatsov commented Mar 18, 2021

Great cop - simple and useful at the same time!

## Summary

This PR adds new `Style/StringChars` cop. The cop checks for uses of
`String#split` with empty string or regexp literal argument.

```ruby
# bad
string.split(//)
string.split('')

# good
string.chars
```

Since Ruby 2.0, the behavior of `String#chars` and `String#split(//)` is the same.

### Ruby 1.9 and lower

```console
% ruby -ve "p 'foo'.split(//)"
ruby 1.9.3p551 (2014-11-13 revision 48407) [x86_64-darwin13.0.2]
["f", "o", "o"]
% ruby -ve "p 'foo'.chars"
ruby 1.9.3p551 (2014-11-13 revision 48407) [x86_64-darwin13.0.2]
#<Enumerator: "foo":chars>
```

### Ruby 2.0 and higher

```console
% ruby -ve "p 'foo'.split(//)"
ruby 2.0.0p648 (2015-12-16 revision 53162) [x86_64-darwin13.0.2]
["f", "o", "o"]
% ruby -ve "p 'foo'.chars"
ruby 2.0.0p648 (2015-12-16 revision 53162) [x86_64-darwin13.0.2]
["f", "o", "o"]
```

Writing `String#chars` would pretty express the intent of the code.

## Additional Information

This cop is intended to use `String#chars` method that express readable code.
And it has a different purpose than `Performance/RedundantSplitRegexpArgument` cop.
https://docs.rubocop.org/rubocop-performance/cops_performance.html#performanceredundantsplitregexpargument

Therefore, it will be added as a new `Style` cop.
@koic koic force-pushed the add_new_style_string_chars_cop branch from c5fe028 to 5fbfe36 Compare March 18, 2021 18:09
@bbatsov bbatsov merged commit 5cd6116 into rubocop:master Mar 18, 2021
@koic koic deleted the add_new_style_string_chars_cop branch March 18, 2021 18:59
koic added a commit to koic/ruby-style-guide that referenced this pull request Mar 19, 2021
Follow rubocop/rubocop#9615.

This PR adds "String#chars" rule.

```ruby
# bad
string.split(//)
string.split('')

# good
string.chars
```
koic added a commit to koic/ruby-style-guide that referenced this pull request Mar 19, 2021
Follow rubocop/rubocop#9615.

This PR adds "String#chars" rule.

```ruby
# bad
string.split(//)
string.split('')

# good
string.chars
```
bbatsov pushed a commit to rubocop/ruby-style-guide that referenced this pull request Mar 19, 2021
Follow rubocop/rubocop#9615.

This PR adds "String#chars" rule.

```ruby
# bad
string.split(//)
string.split('')

# good
string.chars
```
koic added a commit to koic/standard that referenced this pull request Apr 5, 2021
This PR updates RuboCop to 1.12.1.
rubocop/rubocop@v1.11.0...v1.12.1

It supports private API change from `RuboCop::DirectiveComment::DIRECTIVE_COMMENT_REGEXP`
to `RuboCop::DirectiveComment::COMMENT_DIRECTIVE_REGEXP`.

- rubocop/rubocop@7f1f75c
- rubocop/rubocop@0421710

And `Style/StringChars` cop is added in RuboCop 1.12. It is up to maintainers to
decide whether to enable it :-)
rubocop/rubocop#9615
jmkoni pushed a commit to standardrb/standard that referenced this pull request May 3, 2021
* Update rubocop from 1.12.1 to [1.13.0](https://github.com/rubocop-hq/rubocop/releases/tag/v1.13.0)
* Update rubocop-performance from 1.9.2 to [1.11.1](https://github.com/rubocop-hq/rubocop-performance/releases/tag/v1.11.1)
* Enabled the following rules:
  * [`Performance/RedundantSplitRegexpArgument`](rubocop/rubocop-performance#190)
  * [`Style/IfWithBooleanLiteralBranches`](rubocop/rubocop#9396)
  * [`Lint/TripleQuotes`](rubocop/rubocop#9402)
  * [`Lint/SymbolConversion`](rubocop/rubocop#9362)
  * [`Lint/OrAssignmentToConstant`](rubocop/rubocop#9363)
  * [`Lint/NumberedParameterAssignment`](rubocop/rubocop#9326)
  * [`Style/HashConversion`](rubocop/rubocop#9478)
  * [`Gemspec/DateAssignment`](rubocop/rubocop#9496)
  * [`Style/StringChars`](rubocop/rubocop#9615)
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.

None yet

2 participants