Style/ConditionalAssigment fails to autocorrect

[deivid-rodriguez]

Hi! I just tried RuboCop's master branch. It's as awesome as it looks. I found this problem:

example.rb

value.boolean? ? key =~ /#{abbr}/ : key =~ /#{shortcut}/

Running RuboCop on it

deivid@pantani ~/Code/test_app $ bundle exec rubocop example.rb 
Inspecting 1 file
C

Offenses:

example.rb:1:1: C: Use the return of the conditional for variable assignment and comparison.
value.boolean? ? key =~ /#{abbr}/ : key =~ /#{shortcut}/
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

1 file inspected, 1 offense detected

Trying to autocorrect

deivid@pantani ~/Code/test_app $ bundle exec rubocop example.rb -a
Inspecting 1 file
An error occurred while Style/ConditionalAssignment cop was inspecting /home/deivid/Code/test_app/example.rb.
To see the complete backtrace run rubocop -d.
.

1 file inspected, no offenses detected

1 error occurred:
An error occurred while Style/ConditionalAssignment cop was inspecting /home/deivid/Code/test_app/example.rb.
Errors are usually caused by RuboCop bugs.
Please, report your problems to RuboCop's issue tracker.
Mention the following information in the issue report:
0.35.1 (using Parser 2.3.0.pre.6, running on ruby 2.3.0 x86_64-linux)

My bet is that it'll take @alexdowad aproximately 10 minutes to fix it. 😂

Thanks!

[alexdowad]

My bet is that it'll take @alexdowad aproximately 10 minutes to fix it.

Challenge accepted.

Starting my clock... NOW.

[deivid-rodriguez]

Hahahaha, hurry up I've invested a lot of money in this. 😉

[alexdowad]

Hmm. I see that @rrosenblum specifically designed this cop to flag the code you've shown. It wants to see this instead:

key =~ (value.boolean? ? /#{abbr}/ : /#{shortcut}/)

...Which does make sense. Do you agree?

The ConditionalAssignment name is a bit unfortunate, though, seeing that this isn't an assignment.

[deivid-rodriguez]

Sure, makes sense. And the comment about the name too. The autocorrection needs to be fixed though.

[alexdowad]

OK, I blew the 10 minutes here because of misunderstanding the problem. Now let me see why autocorrection is failing.

[deivid-rodriguez]

(And because you fixed another issue in between)

[alexdowad]

OK. TernaryCorrector was written based on the assumption that ConditionalAssignment only corrects... assignments. But it also corrects send nodes in some cases.

[alexdowad]

Here's the problem code:

            condition, if_branch, else_branch = *node
            _if_assignment, if_variable = *if_branch
            _else_assignment, else_variable = *else_branch
            condition = condition.source
            if_variable = if_variable.source

if_variable is :=~, which is not what the author had in mind.

[alexdowad]

Close to nailing this one, but there is still the issue of operator priority. It needs to be smart enough to add parens if necessary.

[deivid-rodriguez]

Don't worry, you still have 4 minutes. :)

[alexdowad]

Issue of operator precedence solved, now writing specs.

[alexdowad]

Wrote specs, now committing and pushing.

[alexdowad]

Pushed fix, waiting to see if CI passes.

[alexdowad]

Thanks for the testing... please find us some more bugs! I'm sure there are plenty left to find and fix.

[deivid-rodriguez]

That was pretty amazing. Thanks so much!

[rrosenblum]

@alexdowad thanks for fixing this. I added on support for ternary operators at the very end. It didn't get quite as much attention as all of the other features.

The ConditionalAssignment name is a bit unfortunate, though, seeing that this isn't an assignment.

I agree, this cop has diverged a bit from its name. The original intention was for it cover =. Then it grew to cover += and -=. Supporting << could also fall under the same name. == and =~ are also supported, but diverged from the assignment portion of the name. If anyone has a suggestion for a different name, I am all for changing it.

Thanks for the testing... please find us some more bugs! I'm sure there are plenty left to find and fix.

Agreed, I am all for finding and fixing bugs.