-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
[Fix #14420] Fix false positives for Style/SafeNavigation
#14421
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 #14420] Fix false positives for Style/SafeNavigation
#14421
Conversation
4300e07 to
da0e3e7
Compare
|
|
||
| it 'registers an offense for ternary expression with operator method call with method chain' do | ||
| expect_offense(<<~RUBY, variable: variable) | ||
| it 'does not register an offense for ternary expression with operator method call with method chain' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be move up in the allowed example, earlier in this context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, how is this different from the behavior above in practical terms?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a spec that described incorrect behavior. As noted in #14420, the previous behavior of this test was to auto-correct from var.nil? ? nil : var.foo * 42 to var&.foo * 42, which is not safe. To make it safe, the correction would have to be var&.foo&.*(42), but that would reduce readability.
This is a more complex version of examples already documented.
# foo ? foo[index] : nil # Ignored `foo&.[](index)` due to unclear readability benefit.
# foo ? foo[idx] = v : nil # Ignored `foo&.[]=(idx, v)` due to unclear readability benefit.
# foo ? foo * 42 : nil # Ignored `foo&.*(42)` due to unclear readability benefit. So the documentation has not been updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant that https://github.com/rubocop/rubocop/pull/14421/files/da0e3e7a1fb6bf65a17f32fcfa3e5e44f9aec36d#diff-736e663e58ab853ebfddd666306d7bddb37502d4cde5c1b8703cac7f1b20dcc8R871 is essentially the same.
And if you go up in the specs group you'll see that the allowed examples are earlier, so I think those should be grouped together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. In cases like foo.*(42) or foo.[](42), where the method call already includes a dot, converting them to safe navigation like foo&.*(42) or foo&.[42] doesn't reduce readability, so an offense is registered.
On the other hand, for calls like foo * 42 or foo[42], which don't include a dot, no offense is registered in order to preserve readability.
So, there's a distinction based on whether operator method call and index access are written with a dot.
I structured the specs to reflect this symmetry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I misunderstood the intent. I've updated it to keep the diff simple.
da0e3e7 to
837d5a2
Compare
This PR fixes false positives for `Style/SafeNavigation` when ternary expression with operator method call with method chain. Fixes rubocop#14420.
837d5a2 to
c9ecf16
Compare
This PR fixes false positives for
Style/SafeNavigationwhen ternary expression with operator method call with method chain.Fixes #14420.
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.