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 #9731] Fix two autocorrection issues for Style/NegatedIfElseCondition #9756

Merged
merged 1 commit into from May 2, 2021
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_two_autocorrection_issues_for.md
@@ -0,0 +1 @@
* [#9731](https://github.com/rubocop/rubocop/issues/9731): Fix two autocorrection issues for `Style/NegatedIfElseCondition`. ([@dvandersluis][])
26 changes: 17 additions & 9 deletions lib/rubocop/cop/style/negated_if_else_condition.rb
Expand Up @@ -29,7 +29,6 @@ module Style
#
class NegatedIfElseCondition < Base
include RangeHelp
include CommentsHelp
extend AutoCorrector

MSG = 'Invert the negated condition and swap the %<type>s branches.'
Expand Down Expand Up @@ -98,21 +97,30 @@ def swap_branches(corrector, node)
if node.if_branch.nil?
corrector.remove(range_by_whole_lines(node.loc.else, include_final_newline: true))
else
if_range = node_with_comments(node.if_branch)
else_range = node_with_comments(node.else_branch)
if_range = if_range(node)
else_range = else_range(node)

corrector.replace(if_range, else_range.source)
corrector.replace(else_range, if_range.source)
end
end

def node_with_comments(node)
first_statement = node.begin_type? ? node.children[0] : node
return node if processed_source.ast_with_comments[first_statement].empty?
# Collect the entire if branch, including whitespace and comments
def if_range(node)
if node.ternary?
node.if_branch
else
range_between(node.condition.loc.expression.end_pos, node.loc.else.begin_pos)
end
end

begin_pos = source_range_with_comment(first_statement).begin_pos
end_pos = node.source_range.end_pos
Parser::Source::Range.new(buffer, begin_pos, end_pos)
# Collect the entire else branch, including whitespace and comments
def else_range(node)
if node.ternary?
node.else_branch
else
range_between(node.loc.else.end_pos, node.loc.end.begin_pos)
end
end
end
end
Expand Down
124 changes: 124 additions & 0 deletions spec/rubocop/cop/style/negated_if_else_condition_spec.rb
Expand Up @@ -50,6 +50,21 @@
RUBY
end

it 'registers an offens and corrects a multiline ternary' do
expect_offense(<<~RUBY)
!x ?
^^^^ Invert the negated condition and swap the ternary branches.
do_something :
do_something_else # comment
RUBY

expect_correction(<<~RUBY)
x ?
do_something_else :
do_something # comment
RUBY
end
Comment on lines +53 to +66
Copy link
Member Author

Choose a reason for hiding this comment

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

This doesn't actually correct the comment properly but I wanted to add a test for this case just to document behaviour. Really no one should be writing multiline ternaries like this. 😏


shared_examples 'negation method' do |method, inverted_method|
it "registers an offense and corrects when negating condition with `#{method}` for `if-else`" do
expect_offense(<<~RUBY, method: method)
Expand Down Expand Up @@ -129,6 +144,23 @@
RUBY
end

it 'does not register an offense when the `else` branch is empty' do
expect_no_offenses(<<~RUBY)
if !condition.nil?
foo = 42
else
end
RUBY
end

it 'does not register an offense when both branches are empty' do
expect_no_offenses(<<~RUBY)
if !condition.nil?
else
end
RUBY
end

it 'moves comments to correct branches during autocorrect' do
expect_offense(<<~RUBY)
if !condition.nil?
Expand Down Expand Up @@ -187,6 +219,98 @@
RUBY
end

it 'works with comments when one branch is a begin and the other is not' do
expect_offense(<<~RUBY)
if !condition
^^^^^^^^^^^^^ Invert the negated condition and swap the if-else branches.
# comment
do_a
do_b
else
do_c
end
RUBY

expect_correction(<<~RUBY)
if condition
do_c
else
# comment
do_a
do_b
end
RUBY
end

it 'works with comments when neither branch is a begin node' do
expect_offense(<<~RUBY)
if !condition
^^^^^^^^^^^^^ Invert the negated condition and swap the if-else branches.
# comment
do_b
else
do_c
end
RUBY

expect_correction(<<~RUBY)
if condition
do_c
else
# comment
do_b
end
RUBY
end

it 'works with duplicate nodes' do
expect_offense(<<~RUBY)
# outer comment
do_a

if !condition
^^^^^^^^^^^^^ Invert the negated condition and swap the if-else branches.
# comment
do_a
else
do_c
end
RUBY

expect_correction(<<~RUBY)
# outer comment
do_a

if condition
do_c
else
# comment
do_a
end
RUBY
end

it 'correctly moves comments at the end of branches' do
expect_offense(<<~RUBY)
if !condition
^^^^^^^^^^^^^ Invert the negated condition and swap the if-else branches.
do_a
# comment
else
do_c
end
RUBY

expect_correction(<<~RUBY)
if condition
do_c
else
do_a
# comment
end
RUBY
end

it 'does not register an offense when negating condition for `if-elsif`' do
expect_no_offenses(<<~RUBY)
if !x
Expand Down