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 #9144] Add aggressive and conservative enforced styles for Style/StringConcatenation cop #9153

Merged
merged 1 commit into from Jun 28, 2021
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog/change_string_concatenation_enforced_styles.md
@@ -0,0 +1 @@
* [#9144](https://github.com/rubocop/rubocop/issues/9144): Add `aggressive` and `conservative` modes of operation for `Style/StringConcatenation` cop. ([@tejasbubane][])
3 changes: 2 additions & 1 deletion config/default.yml
Expand Up @@ -4620,7 +4620,8 @@ Style/StringConcatenation:
Enabled: true
Safe: false
VersionAdded: '0.89'
VersionChanged: '1.6'
VersionChanged: <<next>>
Mode: aggressive

Style/StringHashKeys:
Description: 'Prefer symbols instead of strings as hash keys.'
Expand Down
37 changes: 32 additions & 5 deletions lib/rubocop/cop/style/string_concatenation.rb
Expand Up @@ -15,18 +15,37 @@ module Style
# lines, this cop does not register an offense; instead,
# `Style/LineEndConcatenation` will pick up the offense if enabled.
#
# @example
# Two modes are supported:
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should also mention how this ties to the cop safety - e.g. with the Pathname example.

# 1. `aggressive` style checks and corrects all occurrences of `+` where
# either the left or right side of `+` is a string literal.
# 2. `conservative` style on the other hand, checks and corrects only if
# left side (receiver of `+` method call) is a string literal.
# This is useful when the receiver is some expression that returns string like `Pathname`
Copy link
Contributor

Choose a reason for hiding this comment

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

Pathname.new returns a Pathname, not a string. Pathname.new("x") + "y" also returns a Pathname.

I think this comment should read

This is useful when the receiver is an object whose + operator accepts a string, but does not return a string -- such as Pathname

# instead of a string literal.
#
# @example Mode: aggressive (default)
# # bad
# email_with_name = user.name + ' <' + user.email + '>'
# Pathname.new('/') + 'test'
#
# # good
# email_with_name = "#{user.name} <#{user.email}>"
# email_with_name = format('%s <%s>', user.name, user.email)
# "#{Pathname.new('/')}test"
#
# # accepted, line-end concatenation
# name = 'First' +
# 'Last'
#
# @example Mode: conservative
# # bad
# 'Hello' + user.name
#
# # good
# "Hello #{user.name}"
# user.name + '!!'
# Pathname.new('/') + 'test'
#
class StringConcatenation < Base
include Util
include RangeHelp
Expand All @@ -52,10 +71,15 @@ def on_send(node)
return if line_end_concatenation?(node)

topmost_plus_node = find_topmost_plus_node(node)
parts = collect_parts(topmost_plus_node)
return unless parts[0..-2].any? { |receiver_node| offensive_for_mode?(receiver_node) }

parts = []
collect_parts(topmost_plus_node, parts)
register_offense(topmost_plus_node, parts)
end

private

def register_offense(topmost_plus_node, parts)
add_offense(topmost_plus_node) do |corrector|
correctable_parts = parts.none? { |part| uncorrectable?(part) }
if correctable_parts && !corrected_ancestor?(topmost_plus_node)
Expand All @@ -67,7 +91,10 @@ def on_send(node)
end
end

private
def offensive_for_mode?(receiver_node)
mode = cop_config['Mode'].to_sym
mode == :aggressive || mode == :conservative && receiver_node.str_type?
end

def line_end_concatenation?(node)
# If the concatenation happens at the end of the line,
Expand All @@ -87,7 +114,7 @@ def find_topmost_plus_node(node)
current
end

def collect_parts(node, parts)
def collect_parts(node, parts = [])
return unless node

if plus_node?(node)
Expand Down
32 changes: 32 additions & 0 deletions spec/rubocop/cop/style/string_concatenation_spec.rb
Expand Up @@ -203,4 +203,36 @@
RUBY
end
end

context 'Mode = conservative' do
let(:cop_config) { { 'Mode' => 'conservative' } }

context 'when first operand is not string literal' do
it 'does not register offense' do
expect_no_offenses(<<~RUBY)
user.name + "!!"
user.name + "<"
RUBY
end
end

context 'when first operand is string literal' do
it 'registers offense' do
expect_offense(<<~RUBY)
"Hello " + user.name
^^^^^^^^^^^^^^^^^^^^ Prefer string interpolation to string concatenation.
"Hello " + user.name + "!!"
^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer string interpolation to string concatenation.
user.name + "<" + "user.email" + ">"
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer string interpolation to string concatenation.
RUBY

expect_correction(<<~RUBY)
"Hello \#{user.name}"
"Hello \#{user.name}!!"
"\#{user.name}<user.email>"
RUBY
end
end
end
end