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.

Sign up for GitHub

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

Jump to bottom

Cop for deterministic regexp on String#split #190

Merged
merged 1 commit into from
Jan 22, 2021
Merged

Cop for deterministic regexp on String#split #190

merged 1 commit into from
Jan 22, 2021

Conversation

mfbmina
Copy link
Contributor

@mfbmina mfbmina commented Nov 7, 2020

This feature was requested here: https://github.com/rubocop-hq/rubocop/issues/8979

Sometimes, a regexp is used when a simple string could solve the problem, this PR looks for that when calling .split.

Before submitting the PR make sure the following are checked:

  • 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.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.
  • Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

koic
koic reviewed Nov 8, 2020
View reviewed changes
config/default.yml Outdated Show resolved Hide resolved
koic
koic reviewed Nov 8, 2020
View reviewed changes
config/default.yml Outdated Show resolved Hide resolved
koic
koic reviewed Nov 8, 2020
View reviewed changes
config/default.yml Outdated Show resolved Hide resolved
@koic
Copy link
Member

koic commented Nov 8, 2020

I came up with a cop name RedundantSplitRegexpArgument. But there seems to be a better name, however I can't hatch of it yet 😅

@mfbmina
Copy link
Contributor Author

mfbmina commented Nov 8, 2020

Hello @koic! I applied all the required changes! I also don't have a better name than that 😅 but I like yours better.

@mfbmina mfbmina requested a review from koic November 8, 2020 19:44
@tas50
Copy link
Contributor

tas50 commented Nov 9, 2020

Testing this out against my codebase it looks like it's matching on this code:

filter_parts = filter.split(/\./)

This is being autocorrected to:

filter_parts = filter.split('\.')

This should be autocorrected to:

filter_parts = filter.split('.')

koic
koic reviewed Nov 9, 2020
View reviewed changes
config/default.yml Outdated Show resolved Hide resolved
koic
koic reviewed Nov 9, 2020
View reviewed changes
config/default.yml Outdated Show resolved Hide resolved
@mfbmina
Copy link
Contributor Author

mfbmina commented Nov 9, 2020 •
edited
Loading

@tas50 I couldn't reproduce your issue. Your case is not considered an offense and it will not be auto-corrected.

I also added a test case for it: https://github.com/rubocop-hq/rubocop-performance/pull/190/files#diff-bbc2461ef6fae3f431f7a7dbc2b3852a9bcf830680f583806a7429e2e414a830R14-R17

@mfbmina mfbmina requested a review from koic November 10, 2020 17:45
@mfbmina mfbmina requested a review from koic November 18, 2020 19:05
@mfbmina mfbmina requested a review from koic November 24, 2020 15:59
koic
koic reviewed Nov 25, 2020
View reviewed changes
config/default.yml Outdated Show resolved Hide resolved
@koic
Copy link
Member

koic commented Dec 18, 2020

Can you add the changelog entry to CHANGELOG.md and squash your commits into one?

@mfbmina
Copy link
Contributor Author

mfbmina commented Dec 28, 2020

@koic hello! sorry for taking long to respond to you! I've applied the necessary changes. Please review

@mfbmina
Copy link
Contributor Author

mfbmina commented Jan 14, 2021

Sorry, @koic!

I've missed this: Can you add the changelog entry to CHANGELOG.md and squash your commits into one?

It is done now!

@mfbmina @tas50
Add new Performance/RedundantSplitRegexpArgument cop.
5f3d528 
Apply reviews

Remove wrong configuration

Fix auto-correction with escaping chars

Ignore string special chars

Guarantee string will behave the same when using special chars

Apply review

Add changelog

Update docs/modules/ROOT/pages/cops_performance.adoc

Co-authored-by: Tim Smith <[email protected]>
@mfbmina mfbmina requested a review from koic January 14, 2021 22:51
@mfbmina
Copy link
Contributor Author

mfbmina commented Jan 21, 2021

@koic @tas50 is something still missing?

@tas50
Copy link
Contributor

tas50 commented Jan 21, 2021

@mfbmina I'm happy with it.

@koic
Copy link
Member

koic commented Jan 22, 2021

It looks good overall. I will tweak the changelog after merging. Thank you for implementing the useful cop!

@koic koic merged commit d0cc003 into rubocop:master Jan 22, 2021
koic added a commit that referenced this pull request Jan 22, 2021
@koic
Tweak the changelog
209a88b 
Follow #190 (comment)
@mfbmina
Copy link
Contributor Author

mfbmina commented Jan 22, 2021

Thank you guys 😄

@mfbmina mfbmina deleted the regexp-in-split branch January 22, 2021 08:24
koic added a commit to koic/rubocop-performance that referenced this pull request Jan 25, 2021
@koic
Fix an incorrect auto-correct for `Performance/RedundantSplitRegexpAr…
aabe096 
…gument`

Follow rubocop#190.

This PR fixes the following incorrect auto-correct for `Performance/RedundantSplitRegexpArgument`
when using consecutive special string chars.

```console
% cat example.rb
"foo\nbar\nbaz\n".split(/\n\n/)

% bundle exec rubocop example.rb --only Performance/RedundantSplitRegexpArgument -a
Inspecting 1 file
C

Offenses:

example.rb:1:25: C: [Corrected]
Performance/RedundantSplitRegexpArgument: Use string as argument instead of regexp.
"foo\nbar\nbaz\n".split(/\n\n/)
                        ^^^^^^

1 file inspected, 1 offense detected, 1 offense corrected

## Before

```console
% cat example.rb
"foo\nbar\nbaz\n".split("nn")
```

## After

```console
% cat example.rb
"foo\nbar\nbaz\n".split("\n\n")
```
@koic koic mentioned this pull request Jan 25, 2021
Merged
8 tasks
koic added a commit to koic/rubocop-performance that referenced this pull request Jan 26, 2021
@koic
Fix an incorrect auto-correct for `Performance/RedundantSplitRegexpAr…
5b2753e 
…gument`

Follow rubocop#190.

This PR fixes the following incorrect auto-correct for `Performance/RedundantSplitRegexpArgument`
when using consecutive special string chars.

```console
% cat example.rb
"foo\nbar\nbaz\n".split(/\n\n/)

% bundle exec rubocop example.rb --only Performance/RedundantSplitRegexpArgument -a
Inspecting 1 file
C

Offenses:

example.rb:1:25: C: [Corrected]
Performance/RedundantSplitRegexpArgument: Use string as argument instead of regexp.
"foo\nbar\nbaz\n".split(/\n\n/)
                        ^^^^^^

1 file inspected, 1 offense detected, 1 offense corrected

## Before

```console
% cat example.rb
"foo\nbar\nbaz\n".split("nn")
```

## After

```console
% cat example.rb
"foo\nbar\nbaz\n".split("\n\n")
```
koic added a commit to koic/rubocop-performance that referenced this pull request Jan 26, 2021
@koic
Fix an incorrect auto-correct for `Performance/RedundantSplitRegexpAr…
44289a7 
…gument`

Follow rubocop#190.

This PR fixes the following incorrect auto-correct for `Performance/RedundantSplitRegexpArgument`
when using consecutive special string chars.

```console
% cat example.rb
"foo\nbar\nbaz\n".split(/\n\n/)

% bundle exec rubocop example.rb --only Performance/RedundantSplitRegexpArgument -a
Inspecting 1 file
C

Offenses:

example.rb:1:25: C: [Corrected]
Performance/RedundantSplitRegexpArgument: Use string as argument instead of regexp.
"foo\nbar\nbaz\n".split(/\n\n/)
                        ^^^^^^

1 file inspected, 1 offense detected, 1 offense corrected

## Before

```console
% cat example.rb
"foo\nbar\nbaz\n".split("nn")
```

## After

```console
% cat example.rb
"foo\nbar\nbaz\n".split("\n\n")
```
koic added a commit to koic/rubocop-performance that referenced this pull request Jan 26, 2021
@koic
Fix an incorrect auto-correct for `Performance/RedundantSplitRegexpAr…
f0464c9 
…gument`

Follow rubocop#190.

This PR fixes the following incorrect auto-correct for `Performance/RedundantSplitRegexpArgument`
when using consecutive special string chars.

```console
% cat example.rb
"foo\nbar\nbaz\n".split(/\n\n/)

% bundle exec rubocop example.rb --only Performance/RedundantSplitRegexpArgument -a
Inspecting 1 file
C

Offenses:

example.rb:1:25: C: [Corrected]
Performance/RedundantSplitRegexpArgument: Use string as argument instead of regexp.
"foo\nbar\nbaz\n".split(/\n\n/)
                        ^^^^^^

1 file inspected, 1 offense detected, 1 offense corrected

## Before

```console
% cat example.rb
"foo\nbar\nbaz\n".split("nn")
```

## After

```console
% cat example.rb
"foo\nbar\nbaz\n".split("\n\n")
```
@dependabot dependabot bot mentioned this pull request Mar 16, 2021
Merged
jmkoni pushed a commit to standardrb/standard that referenced this pull request May 3, 2021
Update rubocop and rubocop-performance and finally add in some new cops
2ec5998 
* 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)
@jmkoni jmkoni mentioned this pull request May 3, 2021
Merged
patrickm53 pushed a commit to patrickm53/performance-develop-rubyonrails that referenced this pull request Sep 23, 2022
Tweak the changelog
3207cd6 
Follow rubocop/rubocop-performance#190 (comment)
richardstewart0213 added a commit to richardstewart0213/performance-build-Performance-optimization-analysis- that referenced this pull request Nov 4, 2022
@richardstewart0213
Tweak the changelog
6588756 
Follow rubocop/rubocop-performance#190 (comment)
Cute0110 added a commit to Cute0110/Rubocop-Performance that referenced this pull request Sep 28, 2023
@Cute0110
Tweak the changelog
108221c 
Follow rubocop/rubocop-performance#190 (comment)
SerhiiMisiura added a commit to SerhiiMisiura/Rubocop-Performance that referenced this pull request Oct 5, 2023
@SerhiiMisiura
Tweak the changelog
1a82583 
Follow rubocop/rubocop-performance#190 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tas50 tas50 tas50 left review comments

@koic koic Awaiting requested review from koic

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mfbmina @koic @tas50