From 9c6bd9ee9f14f03a5dca68769039ef3b8f599789 Mon Sep 17 00:00:00 2001 From: Bernhard Brodowsky Date: Wed, 12 Feb 2020 22:28:16 +0100 Subject: [PATCH] [Fix #7705] Fix problem of one_line_conditional cop with elsif. 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 #7705 for more details. --- CHANGELOG.md | 2 + lib/rubocop/cop/style/one_line_conditional.rb | 52 ++++++++++++++++--- manual/cops_naming.md | 2 +- .../cop/style/one_line_conditional_spec.rb | 43 +++++++++++++++ 4 files changed, 90 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 813b33fe835..875171ba6c0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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][]) @@ -4357,3 +4358,4 @@ [@masarakki]: https://github.com/masarakki [@djudd]: https://github.com/djudd [@jemmaissroff]: https://github.com/jemmaissroff +[@Lykos]: https://github.com/Lykos diff --git a/lib/rubocop/cop/style/one_line_conditional.rb b/lib/rubocop/cop/style/one_line_conditional.rb index 0462da7997b..931469878ed 100644 --- a/lib/rubocop/cop/style/one_line_conditional.rb +++ b/lib/rubocop/cop/style/one_line_conditional.rb @@ -25,11 +25,13 @@ module Style class OneLineConditional < Cop include OnNormalIfUnless - MSG = 'Favor the ternary operator (`?:`) ' \ - 'over `%s/then/else/end` constructs.' + MSG_USE_TERNARY = 'Favor the ternary operator (`?:`) ' \ + 'over `%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 @@ -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) diff --git a/manual/cops_naming.md b/manual/cops_naming.md index 61b06e6e56f..e0122b5eb92 100644 --- a/manual/cops_naming.md +++ b/manual/cops_naming.md @@ -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 diff --git a/spec/rubocop/cop/style/one_line_conditional_spec.rb b/spec/rubocop/cop/style/one_line_conditional_spec.rb index d91b1b3ca9f..66829423d27 100644 --- a/spec/rubocop/cop/style/one_line_conditional_spec.rb +++ b/spec/rubocop/cop/style/one_line_conditional_spec.rb @@ -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) @@ -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' }