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

Let Performance/StartWith and Performance/EndWith correct Regexp#match and Regexp#=~ #82

Merged
merged 1 commit into from
Oct 28, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## master (unreleased)

### Bug fixes

* [#82](https://github.com/rubocop-hq/rubocop-performance/pull/82): Let `Performance/StartWith` and `Performance/EndWith` correct `Regexp#match?` and `Regexp#=~`. ([@eugeneius][])

## 1.5.0 (2019-10-01)

### Bug fixes
Expand Down Expand Up @@ -63,3 +67,4 @@
[@tejasbubane]: https://github.com/tejasbubane
[@rrosenblum]: https://github.com/rrosenblum
[@splattael]: https://github.com/splattael
[@eugeneius]: https://github.com/eugeneius
7 changes: 6 additions & 1 deletion lib/rubocop/cop/performance/end_with.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,11 @@ module Performance
# @example
# # bad
# 'abc'.match?(/bc\Z/)
# /bc\Z/.match?('abc')
# 'abc' =~ /bc\Z/
# /bc\Z/ =~ 'abc'
# 'abc'.match(/bc\Z/)
# /bc\Z/.match('abc')
#
# # good
# 'abc'.end_with?('bc')
Expand All @@ -21,7 +24,8 @@ class EndWith < Cop

def_node_matcher :redundant_regex?, <<-PATTERN
{(send $!nil? {:match :=~ :match?} (regexp (str $#literal_at_end?) (regopt)))
(send (regexp (str $#literal_at_end?) (regopt)) {:match :=~} $_)}
(send (regexp (str $#literal_at_end?) (regopt)) {:match :match?} $_)
(match-with-lvasgn (regexp (str $#literal_at_end?) (regopt)) $_)}
PATTERN

def literal_at_end?(regex_str)
Expand All @@ -36,6 +40,7 @@ def on_send(node)

add_offense(node)
end
alias on_match_with_lvasgn on_send

def autocorrect(node)
redundant_regex?(node) do |receiver, regex_str|
Expand Down
7 changes: 6 additions & 1 deletion lib/rubocop/cop/performance/start_with.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,11 @@ module Performance
# @example
# # bad
# 'abc'.match?(/\Aab/)
# /\Aab/.match?('abc')
# 'abc' =~ /\Aab/
# /\Aab/ =~ 'abc'
# 'abc'.match(/\Aab/)
# /\Aab/.match('abc')
#
# # good
# 'abc'.start_with?('ab')
Expand All @@ -21,7 +24,8 @@ class StartWith < Cop

def_node_matcher :redundant_regex?, <<-PATTERN
{(send $!nil? {:match :=~ :match?} (regexp (str $#literal_at_start?) (regopt)))
(send (regexp (str $#literal_at_start?) (regopt)) {:match :=~} $_)}
(send (regexp (str $#literal_at_start?) (regopt)) {:match :match?} $_)
(match-with-lvasgn (regexp (str $#literal_at_start?) (regopt)) $_)}
PATTERN

def literal_at_start?(regex_str)
Expand All @@ -39,6 +43,7 @@ def on_send(node)

add_offense(node)
end
alias on_match_with_lvasgn on_send

def autocorrect(node)
redundant_regex?(node) do |receiver, regex_str|
Expand Down
6 changes: 6 additions & 0 deletions manual/cops_performance.md
Original file line number Diff line number Diff line change
Expand Up @@ -301,8 +301,11 @@ would suffice.
```ruby
# bad
'abc'.match?(/bc\Z/)
/bc\Z/.match?('abc')
'abc' =~ /bc\Z/
/bc\Z/ =~ 'abc'
'abc'.match(/bc\Z/)
/bc\Z/.match('abc')

# good
'abc'.end_with?('bc')
Expand Down Expand Up @@ -752,8 +755,11 @@ This cop identifies unnecessary use of a regex where
```ruby
# bad
'abc'.match?(/\Aab/)
/\Aab/.match?('abc')
'abc' =~ /\Aab/
/\Aab/ =~ 'abc'
'abc'.match(/\Aab/)
/\Aab/.match('abc')

# good
'abc'.start_with?('ab')
Expand Down
71 changes: 61 additions & 10 deletions spec/rubocop/cop/performance/end_with_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,68 +4,119 @@
subject(:cop) { described_class.new }

shared_examples 'different match methods' do |method|
it "autocorrects #{method} /abc\\z/" do
it "autocorrects str#{method} /abc\\z/" do
new_source = autocorrect_source("str#{method} /abc\\z/")
expect(new_source).to eq "str.end_with?('abc')"
end

it "autocorrects #{method} /\\n\\z/" do
it "autocorrects /abc\\z/#{method} str" do
new_source = autocorrect_source("/abc\\z/#{method} str")
expect(new_source).to eq "str.end_with?('abc')"
end

it "autocorrects str#{method} /\\n\\z/" do
new_source = autocorrect_source("str#{method} /\\n\\z/")
expect(new_source).to eq 'str.end_with?("\n")'
end

it "autocorrects #{method} /\\t\\z/" do
it "autocorrects /\\n\\z/#{method} str" do
new_source = autocorrect_source("/\\n\\z/#{method} str")
expect(new_source).to eq 'str.end_with?("\n")'
end

it "autocorrects str#{method} /\\t\\z/" do
new_source = autocorrect_source("str#{method} /\\t\\z/")
expect(new_source).to eq 'str.end_with?("\t")'
end

it "autocorrects /\\t\\z/#{method} str" do
new_source = autocorrect_source("/\\t\\z/#{method} str")
expect(new_source).to eq 'str.end_with?("\t")'
end

# regexp metacharacters
%w[. $ ^ |].each do |str|
it "autocorrects #{method} /\\#{str}\\z/" do
it "autocorrects str#{method} /\\#{str}\\z/" do
new_source = autocorrect_source("str#{method} /\\#{str}\\z/")
expect(new_source).to eq "str.end_with?('#{str}')"
end

it "doesn't register an error for #{method} /#{str}\\z/" do
it "autocorrects /\\#{str}\\z/#{method} str" do
new_source = autocorrect_source("/\\#{str}\\z/#{method} str")
expect(new_source).to eq "str.end_with?('#{str}')"
end

it "doesn't register an error for str#{method} /#{str}\\z/" do
expect_no_offenses("str#{method} /#{str}\\z/")
end

it "doesn't register an error for /#{str}\\z/#{method} str" do
expect_no_offenses("/#{str}\\z/#{method} str")
end
end

# escapes like "\n"
# note that "\b" is a literal backspace char in a double-quoted string...
# but in a regex, it's an anchor on a word boundary
%w[a e f r t v].each do |str|
it "autocorrects #{method} /\\#{str}\\z/" do
it "autocorrects str#{method} /\\#{str}\\z/" do
new_source = autocorrect_source("str#{method} /\\#{str}\\z/")
expect(new_source).to eq %{str.end_with?("\\#{str}")}
end

it "autocorrects /\\#{str}\\z/#{method} str" do
new_source = autocorrect_source("/\\#{str}\\z/#{method} str")
expect(new_source).to eq %{str.end_with?("\\#{str}")}
end
end

# character classes, anchors
%w[w W s S d D A Z z G b B h H R X S].each do |str|
it "doesn't register an error for #{method} /\\#{str}\\z/" do
it "doesn't register an error for str#{method} /\\#{str}\\z/" do
expect_no_offenses("str#{method} /\\#{str}\\z/")
end

it "doesn't register an error for /\\#{str}\\z/#{method} str" do
expect_no_offenses("/\\#{str}\\z/#{method} str")
end
end

# characters with no special meaning whatsoever
%w[i j l m o q y].each do |str|
it "autocorrects #{method} /\\#{str}\\z/" do
it "autocorrects str#{method} /\\#{str}\\z/" do
new_source = autocorrect_source("str#{method} /\\#{str}\\z/")
expect(new_source).to eq "str.end_with?('#{str}')"
end

it "autocorrects /\\#{str}\\z/#{method} str" do
new_source = autocorrect_source("/\\#{str}\\z/#{method} str")
expect(new_source).to eq "str.end_with?('#{str}')"
end
end

it "formats the error message correctly for #{method} /abc\\z/" do
it "formats the error message correctly for str#{method} /abc\\z/" do
inspect_source("str#{method} /abc\\z/")
expect(cop.messages).to eq(['Use `String#end_with?` instead of a ' \
'regex match anchored to the end of ' \
'the string.'])
end

it "autocorrects #{method} /\\\\\\z/" do
it "formats the error message correctly for /abc\\z/#{method} str" do
inspect_source("/abc\\z/#{method} str")
expect(cop.messages).to eq(['Use `String#end_with?` instead of a ' \
'regex match anchored to the end of ' \
'the string.'])
end

it "autocorrects str#{method} /\\\\\\z/" do
new_source = autocorrect_source("str#{method} /\\\\\\z/")
expect(new_source).to eq("str.end_with?('\\\\')")
end

it "autocorrects /\\\\\\z/#{method} str" do
new_source = autocorrect_source("/\\\\\\z/#{method} str")
expect(new_source).to eq("str.end_with?('\\\\')")
end
end

include_examples('different match methods', '.match?')
Expand Down
56 changes: 48 additions & 8 deletions spec/rubocop/cop/performance/start_with_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,59 +4,99 @@
subject(:cop) { described_class.new }

shared_examples 'different match methods' do |method|
it "autocorrects #{method} /\\Aabc/" do
it "autocorrects str#{method} /\\Aabc/" do
new_source = autocorrect_source("str#{method} /\\Aabc/")
expect(new_source).to eq "str.start_with?('abc')"
end

it "autocorrects /\\Aabc/#{method} str" do
new_source = autocorrect_source("/\\Aabc/#{method} str")
expect(new_source).to eq "str.start_with?('abc')"
end

# escapes like "\n"
# note that "\b" is a literal backspace char in a double-quoted string...
# but in a regex, it's an anchor on a word boundary
%w[a e f r t v].each do |str|
it "autocorrects #{method} /\\A\\#{str}/" do
it "autocorrects str#{method} /\\A\\#{str}/" do
new_source = autocorrect_source("str#{method} /\\A\\#{str}/")
expect(new_source).to eq %{str.start_with?("\\#{str}")}
end

it "autocorrects /\\A\\#{str}#{method} str/" do
new_source = autocorrect_source("/\\A\\#{str}/#{method} str")
expect(new_source).to eq %{str.start_with?("\\#{str}")}
end
end

# regexp metacharacters
%w[. * ? $ ^ |].each do |str|
it "autocorrects #{method} /\\A\\#{str}/" do
it "autocorrects str#{method} /\\A\\#{str}/" do
new_source = autocorrect_source("str#{method} /\\A\\#{str}/")
expect(new_source).to eq "str.start_with?('#{str}')"
end

it "doesn't register an error for #{method} /\\A#{str}/" do
it "autocorrects /\\A\\#{str}/#{method} str" do
new_source = autocorrect_source("/\\A\\#{str}/#{method} str")
expect(new_source).to eq "str.start_with?('#{str}')"
end

it "doesn't register an error for str#{method} /\\A#{str}/" do
expect_no_offenses("str#{method} /\\A#{str}/")
end

it "doesn't register an error for /\\A#{str}/#{method} str" do
expect_no_offenses("/\\A#{str}/#{method} str")
end
end

# character classes, anchors
%w[w W s S d D A Z z G b B h H R X S].each do |str|
it "doesn't register an error for #{method} /\\A\\#{str}/" do
it "doesn't register an error for str#{method} /\\A\\#{str}/" do
expect_no_offenses("str#{method} /\\A\\#{str}/")
end

it "doesn't register an error for /\\A\\#{str}/#{method} str" do
expect_no_offenses("/\\A\\#{str}/#{method} str")
end
end

# characters with no special meaning whatsoever
%w[i j l m o q y].each do |str|
it "autocorrects #{method} /\\A\\#{str}/" do
it "autocorrects str#{method} /\\A\\#{str}/" do
new_source = autocorrect_source("str#{method} /\\A\\#{str}/")
expect(new_source).to eq "str.start_with?('#{str}')"
end

it "autocorrects /\\A\\#{str}#{method} str/" do
new_source = autocorrect_source("/\\A\\#{str}/#{method} str")
expect(new_source).to eq "str.start_with?('#{str}')"
end
end

it "formats the error message correctly for #{method} /\\Aabc/" do
it "formats the error message correctly for str#{method} /\\Aabc/" do
inspect_source("str#{method} /\\Aabc/")
expect(cop.messages).to eq(['Use `String#start_with?` instead of a ' \
'regex match anchored to the beginning of ' \
'the string.'])
end

it "autocorrects #{method} /\\A\\\\/" do
it "formats the error message correctly for /\\Aabc/#{method} str" do
inspect_source("/\\Aabc/#{method} str")
expect(cop.messages).to eq(['Use `String#start_with?` instead of a ' \
'regex match anchored to the beginning of ' \
'the string.'])
end

it "autocorrects str#{method} /\\A\\\\/" do
new_source = autocorrect_source("str#{method} /\\A\\\\/")
expect(new_source).to eq("str.start_with?('\\\\')")
end

it "autocorrects /\\A\\\\/#{method} str" do
new_source = autocorrect_source("/\\A\\\\/#{method} str")
expect(new_source).to eq("str.start_with?('\\\\')")
end
end

include_examples('different match methods', '.match?')
Expand Down