[Fix #13378] When removing parens in Style/TernaryParentheses with a send node condition, ensure its arguments are parenthesized
#13381
Conversation
|
|
||
| def condition_send_node(condition) | ||
| return condition if condition.call_type? || condition.defined_type? | ||
| return nil unless condition.begin_type? && condition.child_nodes.length == 1 |
There was a problem hiding this comment.
Is this a necessary process? If so, please add a test for it.
There was a problem hiding this comment.
Not necessary, I'll simplify. Thank you!
| return false unless send_node.call_type? || send_node.defined_type? | ||
|
|
||
| send_node.arguments.any? && | ||
| !send_node.parenthesized? && | ||
| (send_node.dot? || | ||
| send_node.safe_navigation? || | ||
| unparenthesized_method_call?(send_node)) |
There was a problem hiding this comment.
Can you refactor to the following?
| return false unless send_node.call_type? || send_node.defined_type? | |
| send_node.arguments.any? && | |
| !send_node.parenthesized? && | |
| (send_node.dot? || | |
| send_node.safe_navigation? || | |
| unparenthesized_method_call?(send_node)) | |
| return false unless send_node.call_type? || send_node.defined_type? | |
| return false if send_node.arguments.none? || send_node.parenthesized? | |
| send_node.dot? || send_node.safe_navigation? || unparenthesized_method_call?(send_node) |
| RUBY | ||
| end | ||
|
|
||
| it 'registers an offense calling method with non-alphanumeric name' do |
There was a problem hiding this comment.
It seems we could use "operator method" instead of "non-alphanumeric name". e.g.:
| it 'registers an offense calling method with non-alphanumeric name' do | |
| it 'registers an offense calling an operator method with dot or safe navigation' do |
dvandersluis
force-pushed
the
issue/13378
branch
from
81046a9 to
e530531
Compare
|
@koic thanks for the review! ❤️ I've updated with your requested changes. |
|
|
||
| def condition_send_node(condition) | ||
| return condition if node_with_args?(condition) | ||
| return nil unless condition.begin_type? |
There was a problem hiding this comment.
Is this condition (return nil unless condition.begin_type?) necessary as well?
There was a problem hiding this comment.
You're right, since this is called from correct_parenthesized, condition is always going to be a begin node already, so this is unnecessary, as is the first check for a callable node. Updated again.
dvandersluis
force-pushed
the
issue/13378
branch
from
e530531 to
30fe3b1
Compare
|
|
||
| def condition_send_node(condition) | ||
| send_node = condition.child_nodes.last | ||
| send_node if node_with_args?(send_node) |
There was a problem hiding this comment.
Apologies for the repetition feedback, it seems the tests pass even without the if node_with_args? guard condition.
There was a problem hiding this comment.
Yup true, because send_node_args_need_parens? calls node_with_args? too. I refactored away the condition_send_node method now.
…` with a `send` node condition, ensure its arguments are parenthesized.
dvandersluis
force-pushed
the
issue/13378
branch
from
30fe3b1 to
0c5cf48
Compare
|
Thanks! |
2 participants
When the condition to a ternary is a send node with arguments, and
Style/TernaryParenthesesremoves parentheses around that condition, ensure the arguments are themselves parenthesized, because otherwise removing the surrounding parens will change the semantics of the ternary.This also allows a number of test cases to be autocorrected which previously did not have autocorrect.
Fixes #13378.
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.