[Fix #10147] Fix Lint/ElseLayout for else nodes with a single line body
#10192
Conversation
| 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) |
There was a problem hiding this comment.
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).
…body The ElseLayout cop, in spirit, is trying to prevent accidentally using else when a user meant to use elsif based on the fact that they used else in a similar way as elsif (by passing an argument to it). However; the syntax of having a compact single line else statement with no other body is valid and likely shouldn't be flagged as problematic. This commit stops ElseLayout cop from reporting an offense when all the else branch's children are on the same line as the else node itself, while still reporting errors if there are children on other lines in the else branch (which is likely a mistake)
|
I've never seen anyone write |
|
Fair enough. FWIW, this pattern exists for the person who filed the issue #10147 presumably, in the codebase that I work on, as well as the Filewatcher codebase (ref: filewatcher/filewatcher#194). I appreciate you taking the time to review this PR. |
|
Yeah, I have no doubt the pattern exists in some codebases, but we have to optimize for the most common pattern. I can consider a configuration option for this if there's enough demand. Thanks for the PR regardless! |
The ElseLayout cop, in spirit, is trying to prevent accidentally using
else when a user meant to use elsif based on the fact that they used
else in a similar way as elsif (by passing an argument to it).
However; the syntax of having a compact single line else statement with
no other body is valid and likely shouldn't be flagged as problematic.
This commit stops ElseLayout cop from reporting an offense when all the
else branch's children are on the same line as the else node itself,
while still reporting errors if there are children on other lines in the
else branch (which is likely a mistake)
Before submitting the PR make sure the following are checked:
[Fix #issue-number](if the related issue exists).master(if not - rebase it).bundle exec rake default. It executes all tests and runs RuboCop on its own code.{change_type}_{change_description}.mdif the new code introduces user-observable changes. See changelog entry format for details.