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

Style/ConditionalAssigment fails to autocorrect #2608

Closed
deivid-rodriguez opened this Issue Jan 9, 2016 · 16 comments

Comments

Projects
None yet
3 participants
@deivid-rodriguez
Contributor

deivid-rodriguez commented Jan 9, 2016

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

This comment has been minimized.

Show comment
Hide comment
@alexdowad

alexdowad Jan 9, 2016

Contributor

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

Challenge accepted.

Starting my clock... NOW.

Contributor

alexdowad commented Jan 9, 2016

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

Challenge accepted.

Starting my clock... NOW.

@deivid-rodriguez

This comment has been minimized.

Show comment
Hide comment
@deivid-rodriguez

deivid-rodriguez Jan 9, 2016

Contributor

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

Contributor

deivid-rodriguez commented Jan 9, 2016

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

@alexdowad

This comment has been minimized.

Show comment
Hide comment
@alexdowad

alexdowad Jan 9, 2016

Contributor

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.

Contributor

alexdowad commented Jan 9, 2016

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

This comment has been minimized.

Show comment
Hide comment
@deivid-rodriguez

deivid-rodriguez Jan 9, 2016

Contributor

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

Contributor

deivid-rodriguez commented Jan 9, 2016

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

@alexdowad

This comment has been minimized.

Show comment
Hide comment
@alexdowad

alexdowad Jan 9, 2016

Contributor

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

Contributor

alexdowad commented Jan 9, 2016

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

@deivid-rodriguez

This comment has been minimized.

Show comment
Hide comment
@deivid-rodriguez

deivid-rodriguez Jan 9, 2016

Contributor

(And because you fixed another issue in between)

Contributor

deivid-rodriguez commented Jan 9, 2016

(And because you fixed another issue in between)

@alexdowad

This comment has been minimized.

Show comment
Hide comment
@alexdowad

alexdowad Jan 9, 2016

Contributor

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

Contributor

alexdowad commented Jan 9, 2016

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

@alexdowad

This comment has been minimized.

Show comment
Hide comment
@alexdowad

alexdowad Jan 9, 2016

Contributor

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.

Contributor

alexdowad commented Jan 9, 2016

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

This comment has been minimized.

Show comment
Hide comment
@alexdowad

alexdowad Jan 9, 2016

Contributor

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.

Contributor

alexdowad commented Jan 9, 2016

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

This comment has been minimized.

Show comment
Hide comment
@deivid-rodriguez

deivid-rodriguez Jan 9, 2016

Contributor

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

Contributor

deivid-rodriguez commented Jan 9, 2016

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

@alexdowad

This comment has been minimized.

Show comment
Hide comment
@alexdowad

alexdowad Jan 9, 2016

Contributor

Issue of operator precedence solved, now writing specs.

Contributor

alexdowad commented Jan 9, 2016

Issue of operator precedence solved, now writing specs.

@alexdowad

This comment has been minimized.

Show comment
Hide comment
@alexdowad

alexdowad Jan 9, 2016

Contributor

Wrote specs, now committing and pushing.

Contributor

alexdowad commented Jan 9, 2016

Wrote specs, now committing and pushing.

alexdowad added a commit to alexdowad/rubocop that referenced this issue Jan 9, 2016

[Fix #2608] Style/ConditionalAssignment can correct =~ in a ternary e…
…xpression

...as well as aref assignment, <=>, !~, and a couple of other operators which
are implemented as method calls.
@alexdowad

This comment has been minimized.

Show comment
Hide comment
@alexdowad

alexdowad Jan 9, 2016

Contributor

Pushed fix, waiting to see if CI passes.

Contributor

alexdowad commented Jan 9, 2016

Pushed fix, waiting to see if CI passes.

@alexdowad

This comment has been minimized.

Show comment
Hide comment
@alexdowad

alexdowad Jan 9, 2016

Contributor

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

Contributor

alexdowad commented Jan 9, 2016

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

@deivid-rodriguez

This comment has been minimized.

Show comment
Hide comment
@deivid-rodriguez

deivid-rodriguez Jan 9, 2016

Contributor

That was pretty amazing. Thanks so much!

Contributor

deivid-rodriguez commented Jan 9, 2016

That was pretty amazing. Thanks so much!

@rrosenblum

This comment has been minimized.

Show comment
Hide comment
@rrosenblum

rrosenblum Jan 10, 2016

Contributor

@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.

Contributor

rrosenblum commented Jan 10, 2016

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment