[Fix #6751] Prevent Style/OneLineConditional from breaking on retry and break keywords

[Drenmi]

This cop would break on code like:

if true then retry else 7 end
if true then break else 7 end

This was happening because the retry node was expected to be decorated with a decorator that includes the MethodDispatchNode extension.

This change adds a RetryNode- and a BreakNode decorator.

It also fixes an issue where superfluous parentheses were added when the keyword has no arguments:

Before:

true ? (retry) : 7

After:

true ? retry : 7

---

Before submitting the PR make sure the following are checked:

• Wrote good commit messages.
• Commit message starts with [Fix #issue-number] (if the related issue exists).
• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• Added tests.
• Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
• The PR relates to only one subject with a clear title
  and description in grammatically correct, complete sentences.
• Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

[koic]

This looks like a wonderful approach 🌟

However issue #6751 also reports on raise and break. raise seems to work without errors, but break seems to be an error.

raise (no offenses)

% cat raise.rb
if true then raise else 7 end
% rubocop -a --only Style/OneLineConditional raise.rb
Inspecting 1 file
C

Offenses:

raise.rb:1:1: C: [Corrected] Style/OneLineConditional: Favor the ternary operator (?:) over if/then/else/end constructs.
if true then raise else 7 end
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

1 file inspected, 1 offense detected, 1 offense corrected
% g diff raise.rb
diff --git a/raise.rb b/raise.rb
index 519c3b0..69ac23b 100644
--- a/raise.rb
+++ b/raise.rb
@@ -1 +1 @@
-if true then raise else 7 end
+true ? raise : 7

break (offense detected)

% cat break.rb
if true then break else 7 end
% rubocop -a --only Style/OneLineConditional break.rb
Inspecting 1 file


0 files inspected, no offenses detected
undefined method `prefix_not?' for s(:break):RuboCop::AST::Node
/Users/koic/.rbenv/versions/2.6.1/lib/ruby/gems/2.6.0/gems/rubocop-0.64.0/lib/rubocop/cop/style/one_line_conditional.rb:92:in `keyword_with_cha
nged_precedence?'
/Users/koic/.rbenv/versions/2.6.1/lib/ruby/gems/2.6.0/gems/rubocop-0.64.0/lib/rubocop/cop/style/one_line_conditional.rb:80:in `requires_parenth
eses?'

So I think that it would be better to also make BreakNode. WDYT?

[Drenmi]

So I think that it would be better to also make BreakNode. WDYT?

👍

[koic]

It seems that retry is surrounded by parentheses when using auto-correct.

-if true then retry else 7 end
+true ? (retry) : 7

I think that parentheses are unnecessary if possible :-)

[Drenmi]

I think that parentheses are unnecessary if possible :-)

Thanks for a great review! I will have a look. 🙂

[Drenmi]

@koic Thanks for a stellar review! ⭐️ I have addressed all your comments in the PR. I will be on holiday from today until Sunday, so feel free to merge if you're okay with the additions. 🙇

[koic]

I think it would be good to have a mention about break keyword too :-)