Skip to content

Commit

Permalink
[Fix #7130] Skip autocorrect in Style/FormatString if second argume…
Browse files Browse the repository at this point in the history
…nt is variable

For `String#%` is second argument is variable, flag offense but skip
autocorrect. It is not possible for rubocop to know if variable is
array in which case the autocorrect leads to invalid code.

Closes #7130.
  • Loading branch information
tejasbubane authored and bbatsov committed Jun 25, 2019
1 parent 97ff421 commit c897642
Show file tree
Hide file tree
Showing 3 changed files with 53 additions and 3 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
* [#6893](https://github.com/rubocop-hq/rubocop/issues/6893): Handle implicit rescue correctly in `Naming/RescuedExceptionsVariableName`. ([@pocke][], [@anthony-robin][])
* [#7165](https://github.com/rubocop-hq/rubocop/issues/7165): Fix an auto-correct error for `Style/ConditionalAssignment` when without `else` branch'. ([@koic][])
* [#7171](https://github.com/rubocop-hq/rubocop/issues/7171): Fix an error for `Style/SafeNavigation` when using `unless nil?` as a safeguarded'. ([@koic][])
* [#7113](https://github.com/rubocop-hq/rubocop/pull/7113): This PR renames `EnforcedStyle: rails` to `EnabledStyle: outdented_access_modifiers` for `Layout/IndentationConsistency`. ([@koic][])
* [#7130](https://github.com/rubocop-hq/rubocop/pull/7130): Skip autocorrect in `Style/FormatString` if second argument to `String#%` is a variable. ([@tejasbubane][])

### Changes

Expand Down
10 changes: 7 additions & 3 deletions lib/rubocop/cop/style/format_string.rb
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,10 @@ class FormatString < Cop
}
PATTERN

def_node_matcher :variable_argument?, <<-PATTERN
(send {str dstr} :% {send_type? lvar_type?})
PATTERN

def on_send(node)
formatter(node) do |selector|
detected_style = selector == :% ? :percent : selector
Expand All @@ -70,10 +74,10 @@ def method_name(style_name)
end

def autocorrect(node)
lambda do |corrector|
detected_method = node.method_name
return if variable_argument?(node)

case detected_method
lambda do |corrector|
case node.method_name
when :%
autocorrect_from_percent(corrector, node)
when :format, :sprintf
Expand Down
44 changes: 44 additions & 0 deletions spec/rubocop/cop/style/format_string_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,13 @@
RUBY
end

it 'registers an offense for variable argument' do
expect_offense(<<~RUBY)
puts "%f" % a
^ Favor `sprintf` over `String#%`.
RUBY
end

it 'does not register an offense for numbers' do
expect_no_offenses('puts 10 % 4')
end
Expand Down Expand Up @@ -68,6 +75,21 @@
corrected = autocorrect_source('puts x % { a: 10, b: 11 }')
expect(corrected).to eq 'puts sprintf(x, a: 10, b: 11)'
end

it 'does not auto-correct String#% with variable argument' do
source = <<~RUBY
puts "%d" % a
RUBY
expect(autocorrect_source(source)).to eq(source)
end

it 'does not auto-correct String#% with variable argument and assignment' do
source = <<~RUBY
a = something()
puts "%d" % a
RUBY
expect(autocorrect_source(source)).to eq(source)
end
end

context 'when enforced style is format' do
Expand All @@ -94,6 +116,13 @@
RUBY
end

it 'registers an offense for variable argument' do
expect_offense(<<~RUBY)
puts "%f" % a
^ Favor `format` over `String#%`.
RUBY
end

it 'does not register an offense for numbers' do
expect_no_offenses('puts 10 % 4')
end
Expand Down Expand Up @@ -142,6 +171,21 @@
corrected = autocorrect_source('puts x % { a: 10, b: 11 }')
expect(corrected).to eq 'puts format(x, a: 10, b: 11)'
end

it 'does not auto-correct String#% with variable argument' do
source = <<~RUBY
puts "%d" % a
RUBY
expect(autocorrect_source(source)).to eq(source)
end

it 'does not auto-correct String#% with variable argument and assignment' do
source = <<~RUBY
a = something()
puts "%d" % a
RUBY
expect(autocorrect_source(source)).to eq(source)
end
end

context 'when enforced style is percent' do
Expand Down

0 comments on commit c897642

Please sign in to comment.