diff --git a/CHANGELOG.md b/CHANGELOG.md index c3c76b59d1d..000864795c9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ### Bug fixes * [#7719](https://github.com/rubocop-hq/rubocop/issues/7719): Fix `Style/NestedParenthesizedCalls` cop for newline. ([@tejasbubane][]) +* [#7709](https://github.com/rubocop-hq/rubocop/issues/7709): Fix correction of `Style/RedundantCondition` when the else branch contains a range. ([@rrosenblum][]) ## 0.80.0 (2020-02-18) diff --git a/lib/rubocop/cop/style/redundant_condition.rb b/lib/rubocop/cop/style/redundant_condition.rb index bdf4feb9260..a2ee5e71fa5 100644 --- a/lib/rubocop/cop/style/redundant_condition.rb +++ b/lib/rubocop/cop/style/redundant_condition.rb @@ -46,7 +46,7 @@ def on_if(node) def autocorrect(node) lambda do |corrector| if node.ternary? - corrector.replace(range_of_offense(node), '||') + correct_ternary(corrector, node) elsif node.modifier_form? || !node.else_branch corrector.replace(node.source_range, node.if_branch.source) else @@ -90,9 +90,13 @@ def use_if_branch?(else_branch) end def else_source(else_branch) - wrap_else = - else_branch.basic_conditional? && else_branch.modifier_form? - wrap_else ? "(#{else_branch.source})" : else_branch.source + if else_branch.basic_conditional? && + else_branch.modifier_form? || + else_branch.range_type? + "(#{else_branch.source})" + else + else_branch.source + end end def make_ternary_form(node) @@ -106,6 +110,15 @@ def make_ternary_form(node) ternary_form end end + + def correct_ternary(corrector, node) + corrector.replace(range_of_offense(node), '||') + + return unless node.else_branch.range_type? + + corrector.insert_before(node.else_branch.loc.expression, '(') + corrector.insert_after(node.else_branch.loc.expression, ')') + end end end end diff --git a/spec/rubocop/cop/style/redundant_condition_spec.rb b/spec/rubocop/cop/style/redundant_condition_spec.rb index 29ed3bff577..457f421d063 100644 --- a/spec/rubocop/cop/style/redundant_condition_spec.rb +++ b/spec/rubocop/cop/style/redundant_condition_spec.rb @@ -4,7 +4,7 @@ subject(:cop) { described_class.new } context 'when regular condition (if)' do - it 'registers no offense' do + it 'accepts different when the condition does not match the branch' do expect_no_offenses(<<~RUBY) if a b @@ -14,7 +14,7 @@ RUBY end - it 'registers no offense for elsif' do + it 'accepts elsif' do expect_no_offenses(<<~RUBY) if a b @@ -27,201 +27,230 @@ end context 'when condition and if_branch are same' do - it 'registers an offense' do + it 'registers an offense and corrects' do expect_offense(<<~RUBY) if b ^^^^ Use double pipes `||` instead. b else - y(x, - z) + c end RUBY - end - context 'when else_branch is complex' do - it 'registers no offense' do - expect_no_offenses(<<~RUBY) - if b - b - else - c - d - end - RUBY - end - end - - context 'when using elsif branch' do - it 'registers no offense' do - expect_no_offenses(<<~RUBY) - if a - a - elsif cond - d - end - RUBY - end - end - - context 'when using modifier if' do - it 'registers an offense' do - expect_offense(<<~RUBY) - bar if bar - ^^^^^^^^^^ This condition is not needed. - RUBY - end + expect_correction(<<~RUBY) + b || c + RUBY end - context 'when `if` condition and `then` branch are the same ' \ - 'and it has no `else` branch' do - it 'registers an offense' do - expect_offense(<<~RUBY) - if do_something - ^^^^^^^^^^^^^^^ This condition is not needed. - do_something - end - RUBY - end - end + it 'registers an offense and corrects complex one liners' do + expect_offense(<<~RUBY) + if b + ^^^^ Use double pipes `||` instead. + b + else + (c || d) + end + RUBY - context 'when using ternary if in `else` branch' do - it 'registers no offense' do - expect_no_offenses(<<~RUBY) - if a - a - else - b ? c : d - end - RUBY - end + expect_correction(<<~RUBY) + b || (c || d) + RUBY end - end - describe '#autocorrection' do - it 'auto-corrects offense' do - new_source = autocorrect_source(<<~RUBY) + it 'registers an offense and corrects modifier nodes offense' do + expect_offense(<<~RUBY) if b + ^^^^ Use double pipes `||` instead. b else - c + c while d end RUBY - expect(new_source).to eq(<<~RUBY) - b || c + + expect_correction(<<~RUBY) + b || (c while d) RUBY end - it 'auto-corrects multiline sendNode offense' do - new_source = autocorrect_source(<<~RUBY) + it 'registers an offense and corrects multiline nodes' do + expect_offense(<<~RUBY) if b + ^^^^ Use double pipes `||` instead. b else y(x, z) end RUBY - expect(new_source).to eq(<<~RUBY) + + expect_correction(<<~RUBY) b || y(x, z) RUBY end - it 'auto-corrects one-line node offense' do - new_source = autocorrect_source(<<~RUBY) - if b - b - else - (c || d) - end + it 'auto-corrects when using `<<` method higher precedence ' \ + 'than `||` operator' do + expect_offense(<<~RUBY) + ary << if foo + ^^^^^^ Use double pipes `||` instead. + foo + else + bar + end RUBY - expect(new_source).to eq(<<~RUBY) - b || (c || d) + + expect_correction(<<~RUBY) + ary << (foo || bar) RUBY end - it 'auto-corrects modifier nodes offense' do - new_source = autocorrect_source(<<~RUBY) + it 'accepts complex else branches' do + expect_no_offenses(<<~RUBY) if b b else - c while d + c + d end RUBY - expect(new_source).to eq(<<~RUBY) - b || (c while d) - RUBY end - it 'auto-corrects modifer if statements' do - new_source = autocorrect_source('bar if bar') - - expect(new_source).to eq('bar') + it 'accepts an elsif branch' do + expect_no_offenses(<<~RUBY) + if a + a + elsif cond + d + end + RUBY end - it 'auto-corrects when using `<<` method higher precedence ' \ - 'than `||` operator' do - new_source = autocorrect_source(<<~RUBY) - ary << if foo - foo - else - bar - end + it 'registers an offense and corrects when using modifier if' do + expect_offense(<<~RUBY) + bar if bar + ^^^^^^^^^^ This condition is not needed. RUBY - expect(new_source).to eq(<<~RUBY) - ary << (foo || bar) + expect_correction(<<~RUBY) + bar RUBY end - it 'when `if` condition and `then` branch are the same ' \ - 'and it has no `else` branch' do - new_source = autocorrect_source(<<~RUBY) + it 'registers an offense and corrects when `if` condition and `then` ' \ + 'branch are the same and it has no `else` branch' do + expect_offense(<<~RUBY) if do_something + ^^^^^^^^^^^^^^^ This condition is not needed. do_something end RUBY - expect(new_source).to eq(<<~RUBY) + expect_correction(<<~RUBY) do_something RUBY end + + it 'accepts when using ternary if in `else` branch' do + expect_no_offenses(<<~RUBY) + if a + a + else + b ? c : d + end + RUBY + end + + it 'registers an offense and corrects when the else branch ' \ + 'contains an irange' do + expect_offense(<<~RUBY) + if foo + ^^^^^^ Use double pipes `||` instead. + foo + else + 1..2 + end + RUBY + + expect_correction(<<~RUBY) + foo || (1..2) + RUBY + end end end - context 'when ternary expression (?:)' do - it 'registers no offense' do + context 'ternary expression (?:)' do + it 'accepts expressions when the condition and if branch do not match' do expect_no_offenses('b ? d : c') end context 'when condition and if_branch are same' do - it 'registers an offense' do + it 'registers an offense and corrects' do expect_offense(<<~RUBY) b ? b : c ^^^^^ Use double pipes `||` instead. RUBY + + expect_correction(<<~RUBY) + b || c + RUBY end - end - describe '#autocorrection' do - it 'auto-corrects vars' do - new_source = autocorrect_source('a = b ? b : c') - expect(new_source).to eq('a = b || c') + it 'registers an offense and corrects nested vars' do + expect_offense(<<~RUBY) + b.x ? b.x : c + ^^^^^^^ Use double pipes `||` instead. + RUBY + + expect_correction(<<~RUBY) + b.x || c + RUBY end - it 'auto-corrects nested vars' do - new_source = autocorrect_source('b.x ? b.x : c') - expect(new_source).to eq('b.x || c') + it 'registers an offense and corrects class vars' do + expect_offense(<<~RUBY) + @b ? @b : c + ^^^^^^ Use double pipes `||` instead. + RUBY + + expect_correction(<<~RUBY) + @b || c + RUBY end - it 'auto-corrects class vars' do - new_source = autocorrect_source('@b ? @b : c') - expect(new_source).to eq('@b || c') + it 'registers an offense and corrects functions' do + expect_offense(<<~RUBY) + a = b(x) ? b(x) : c + ^^^^^^^^ Use double pipes `||` instead. + RUBY + + expect_correction(<<~RUBY) + a = b(x) || c + RUBY end - it 'auto-corrects functions' do - new_source = autocorrect_source('a = b(x) ? b(x) : c') - expect(new_source).to eq('a = b(x) || c') + it 'registers an offense and corrects when the else branch ' \ + 'contains an irange' do + expect_offense(<<~RUBY) + time_period = updated_during ? updated_during : 2.days.ago..Time.now + ^^^^^^^^^^^^^^^^^^ Use double pipes `||` instead. + RUBY + + expect_correction(<<~RUBY) + time_period = updated_during || (2.days.ago..Time.now) + RUBY + end + + it 'registers an offense and corrects when the else branch ' \ + 'contains an erange' do + expect_offense(<<~RUBY) + time_period = updated_during ? updated_during : 2.days.ago...Time.now + ^^^^^^^^^^^^^^^^^^ Use double pipes `||` instead. + RUBY + + expect_correction(<<~RUBY) + time_period = updated_during || (2.days.ago...Time.now) + RUBY end end end @@ -247,34 +276,21 @@ b end RUBY - end - context 'when unless branch is complex' do - it 'registers no offense' do - expect_no_offenses(<<~RUBY) - unless b - c - d - else - b - end - RUBY - end + expect_correction(<<~RUBY) + b || y(x, z) + RUBY end - end - describe '#autocorrection' do - it 'auto-corrects offense' do - new_source = autocorrect_source(<<~RUBY) + it 'accepts complex unless branches' do + expect_no_offenses(<<~RUBY) unless b c + d else b end RUBY - expect(new_source).to eq(<<~RUBY) - b || c - RUBY end end end