Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Fix #10881] Fix Style/SoleNestedConditional to properly wrap block and csend nodes when necessary #10886

Merged
merged 1 commit into from Aug 9, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog/fix_fix_stylesolenestedconditional_to.md
@@ -0,0 +1 @@
* [#10881](https://github.com/rubocop/rubocop/issues/10881): Fix `Style/SoleNestedConditional` to properly wrap `block` and `csend` nodes when necessary. ([@dvandersluis][])
17 changes: 14 additions & 3 deletions lib/rubocop/cop/style/sole_nested_conditional.rb
Expand Up @@ -148,7 +148,18 @@ def correct_for_basic_condition_style(corrector, node, if_branch, and_operator)
)
corrector.replace(range, and_operator)
corrector.remove(range_by_whole_lines(node.loc.end, include_final_newline: true))
corrector.wrap(if_branch.condition, '(', ')') if wrap_condition?(if_branch.condition)

wrap_condition(corrector, if_branch.condition)
end

def wrap_condition(corrector, condition)
# Handle `send` and `block` nodes that need to be wrapped in parens
# FIXME: autocorrection prevents syntax errors by wrapping the entire node in parens,
# but wrapping the argument list would be a more ergonomic correction.
node_to_check = condition&.block_type? ? condition.send_node : condition
return unless wrap_condition?(node_to_check)

corrector.wrap(condition, '(', ')')
end

def correct_for_outer_condition_modify_form_style(corrector, node, if_branch)
Expand Down Expand Up @@ -207,7 +218,7 @@ def insert_bang_for_and(corrector, node)
end

def require_parentheses?(condition)
condition.send_type? && !condition.arguments.empty? && !condition.parenthesized? &&
condition.call_type? && !condition.arguments.empty? && !condition.parenthesized? &&
!condition.comparison_method?
end

Expand All @@ -219,7 +230,7 @@ def arguments_range(node)

def wrap_condition?(node)
node.and_type? || node.or_type? ||
(node.send_type? && node.arguments.any? && !node.parenthesized?)
(node.call_type? && node.arguments.any? && !node.parenthesized?)
end

def replace_condition(condition)
Expand Down
40 changes: 40 additions & 0 deletions spec/rubocop/cop/style/sole_nested_conditional_spec.rb
Expand Up @@ -663,6 +663,46 @@ def foo
end
RUBY
end

context 'with a `csend` node' do
it 'registers an offense and corrects' do
expect_offense(<<~RUBY)
if foo
if bar&.baz quux
^^ Consider merging nested conditions into outer `if` conditions.
do_something
end
end
RUBY

expect_correction(<<~RUBY)
if foo && (bar&.baz quux)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be more better to correct as follows.

Suggested change
if foo && (bar&.baz quux)
if foo && bar&.baz(quux)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but this is the existing autocorrection for send nodes I'm just extending it to csend and block nodes as well to prevent syntax errors from autocorrection.

Would you be good with accepting this as-is and improving autocorrection in a future PR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I think we can do it separately. It would be useful to leave a FIXME comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added!

do_something
end
RUBY
end
end

context 'with a block' do
it 'registers an offense and corrects' do
expect_offense(<<~RUBY)
if foo
if ok? bar do
^^ Consider merging nested conditions into outer `if` conditions.
do_something
end
end
end
RUBY

expect_correction(<<~RUBY)
if foo && (ok? bar do
do_something
end)
end
Comment on lines +699 to +702
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto :-)

Suggested change
if foo && (ok? bar do
do_something
end)
end
if foo && ok?(bar) do
do_something
end
end

RUBY
end
end
end
end

Expand Down