Skip to content

Commit

Permalink
[Fix rubocop#7705] Fix problem of one_line_conditional cop with elsif.
Browse files Browse the repository at this point in the history
One line conditionals with elsif branches are now transformed into
multiline versions. Before this change, they were broken and autocorrect
produced code that resulted in a syntax error. See Issue rubocop#7705 for more
details.
  • Loading branch information
Lykos committed Feb 12, 2020
1 parent d72591b commit 9c6bd9e
Show file tree
Hide file tree
Showing 4 changed files with 90 additions and 9 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Expand Up @@ -15,6 +15,7 @@

### Bug fixes

* [#7705](https://github.com/rubocop-hq/rubocop/issues/7705): Fix problem of one_line_conditional cop with elsif conditionals. ([@Lykos][])
* [#7644](https://github.com/rubocop-hq/rubocop/issues/7644): Fix patterns with named wildcards in unions. ([@marcandre][])
* [#7639](https://github.com/rubocop-hq/rubocop/pull/7639): Fix logical operator edge case in `omit_parentheses` style of `Style/MethodCallWithArgsParentheses`. ([@gsamokovarov][])
* [#7661](https://github.com/rubocop-hq/rubocop/pull/7661): Fix to return correct info from multi-line regexp. ([@Tietew][])
Expand Down Expand Up @@ -4357,3 +4358,4 @@
[@masarakki]: https://github.com/masarakki
[@djudd]: https://github.com/djudd
[@jemmaissroff]: https://github.com/jemmaissroff
[@Lykos]: https://github.com/Lykos
52 changes: 44 additions & 8 deletions lib/rubocop/cop/style/one_line_conditional.rb
Expand Up @@ -25,11 +25,13 @@ module Style
class OneLineConditional < Cop
include OnNormalIfUnless

MSG = 'Favor the ternary operator (`?:`) ' \
'over `%<keyword>s/then/else/end` constructs.'
MSG_USE_TERNARY = 'Favor the ternary operator (`?:`) ' \
'over `%<keyword>s/then/else/end` constructs.'
MSG_USE_MULTILINE = 'Use multiple lines for ' \
'`if/then/elsif/then/else/end` constructs.'

def on_normal_if_unless(node)
return unless node.single_line? && node.else_branch
return unless node.single_line? && node.else_branch && !node.elsif?

add_offense(node)
end
Expand All @@ -43,21 +45,55 @@ def autocorrect(node)
private

def message(node)
format(MSG, keyword: node.keyword)
if node.elsif_conditional?
MSG_USE_MULTILINE
else
format(MSG_USE_TERNARY, keyword: node.keyword)
end
end

def replacement(node)
return to_ternary(node) unless node.parent
return to_ternary_or_multiline(node) unless node.parent

if %i[and or].include?(node.parent.type)
return "(#{to_ternary(node)})"
return "(#{to_ternary_or_multiline(node)})"
end

if node.parent.send_type? && node.parent.operator_method?
return "(#{to_ternary(node)})"
return "(#{to_ternary_or_multiline(node)})"
end

to_ternary(node)
to_ternary_or_multiline(node)
end

def to_ternary_or_multiline(node)
node.elsif_conditional? ? to_multiline(node) : to_ternary(node)
end

def to_multiline(node)
indentation = ' ' * node.source_range.column
to_indented_multiline(node, indentation)
end

def to_indented_multiline(node, indentation)
if_branch = <<~RUBY
#{node.keyword} #{node.condition.source}
#{indentation} #{node.if_branch.source}
RUBY
else_branch = else_branch_to_multiline(node.else_branch, indentation)
if_branch + else_branch
end

def else_branch_to_multiline(else_branch, indentation)
if else_branch.if_type? && else_branch.elsif?
to_indented_multiline(else_branch, indentation)
else
<<~RUBY.chomp
#{indentation}else
#{indentation} #{else_branch.source}
#{indentation}end
RUBY
end
end

def to_ternary(node)
Expand Down
2 changes: 1 addition & 1 deletion manual/cops_naming.md
Expand Up @@ -350,7 +350,7 @@ This cop can be configured with the EnforcedStyleForLeadingUnderscores
directive. It can be configured to allow for memoized instance variables
prefixed with an underscore. Prefixing ivars with an underscore is a
convention that is used to implicitly indicate that an ivar should not
be set or referenced outside of the memoization method.
be set or referencd outside of the memoization method.
### Examples
Expand Down
43 changes: 43 additions & 0 deletions spec/rubocop/cop/style/one_line_conditional_spec.rb
Expand Up @@ -12,6 +12,15 @@
end
end

shared_examples 'multiline offense' do
it 'registers an offense where multiline should be used' do
inspect_source(source)
expect(cop.messages)
.to eq(['Use multiple lines for ' \
'`if/then/elsif/then/else/end` constructs.'])
end
end

shared_examples 'no offense' do
it 'does not register an offense' do
expect_no_offenses(source)
Expand All @@ -38,6 +47,40 @@
end
end

context 'one line if/then/elsif/then/else/end' do
let(:source) { 'if cond then run elsif elscond then elsrun else dont end' }

include_examples 'multiline offense'
include_examples 'autocorrect', <<~RUBY.chomp
if cond
run
elsif elscond
elsrun
else
dont
end
RUBY
end

context 'one line if/then/elsif/then/else/end with multiple elsifs' do
let(:source) do
'if c0 then r0 elsif c1 then r1 elsif c2 then r2 else r3 end'
end

include_examples 'multiline offense'
include_examples 'autocorrect', <<~RUBY.chomp
if c0
r0
elsif c1
r1
elsif c2
r2
else
r3
end
RUBY
end

context 'one line if/then/else/end when `then` branch has no body' do
let(:source) { 'if cond then else dont end' }

Expand Down

0 comments on commit 9c6bd9e

Please sign in to comment.