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.

Sign up for GitHub

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

Jump to bottom

[Fix #10147] Fix Lint/ElseLayout for else nodes with a single line body #10192

Closed
wants to merge 1 commit into from
Closed

[Fix #10147] Fix Lint/ElseLayout for else nodes with a single line body #10192

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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_lintelselayout_for_else_nodes_with_a.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* [#10147](https://github.com/rubocop/rubocop/issues/10147): Fix `Lint/ElseLayout` for else nodes with a single line body. ([@jkeck][])
9 changes: 9 additions & 0 deletions lib/rubocop/cop/lint/else_layout.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,19 @@ def check_else(node)

return unless first_else
return unless same_line?(first_else, node.loc.else)
return if else_branch_with_no_body?(node)

add_offense(first_else) { |corrector| autocorrect(corrector, node, first_else) }
end

def else_branch_with_no_body?(node)
child_lines = node.else_branch.children.collect do |child|
child.loc.line if child.is_a?(RuboCop::AST::Node)
end.compact

child_lines.push(node.else_branch.loc.line).all?(node.loc.else.line)
Copy link
Author

Choose a reason for hiding this comment

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

This works, but if there is any guidance on better ways to approach what I'm trying to accomplish here I would be happy to update this PR.

This is trying to take an approach somewhat similar to what was described in bullet 3 of #10147 (comment) (I believe), but really just tries to determine if all the children of the else branch are on the same line (thus being the return of the else branch).

end

def autocorrect(corrector, node, first_else)
corrector.insert_after(node.loc.else, "\n")

Expand Down
27 changes: 9 additions & 18 deletions spec/rubocop/cop/lint/else_layout_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,24 +23,6 @@
RUBY
end

it 'registers an offense and corrects for the entire else body being on the same line' do
expect_offense(<<~RUBY)
if something
test
else something_else
^^^^^^^^^^^^^^ Odd `else` layout detected. Did you mean to use `elsif`?
end
RUBY

expect_correction(<<~RUBY)
if something
test
else
something_else
end
RUBY
end

it 'accepts proper else' do
expect_no_offenses(<<~RUBY)
if something
Expand Down Expand Up @@ -140,4 +122,13 @@
if a then b else c end
RUBY
end

it 'does not register an offense when the entire else body is on a single line' do
expect_no_offenses(<<~RUBY)
if something
foo
else something_else
end
RUBY
end
end