Skip to content
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

Lint/Debugger autocorrect doesn't remove conditonals #3400

Closed
jcoyne opened this issue Aug 10, 2016 · 6 comments
Closed

Lint/Debugger autocorrect doesn't remove conditonals #3400

jcoyne opened this issue Aug 10, 2016 · 6 comments
Labels

Comments

@jcoyne
Copy link

jcoyne commented Aug 10, 2016

If you have code like:

byebug if 1 == 2

and you run the autocorrect it will leave:

if 1==2

which causes:

unexpected token $end
(Using Ruby 2.2 parser; configure using TargetRubyVersion parameter, under AllCops)

0.42.0 (using Parser 2.3.1.2, running on ruby 2.3.0 x86_64-darwin14)

@Drenmi
Copy link
Collaborator

Drenmi commented Aug 12, 2016

After playing around with this, it looks like it also indiscriminately auto-corrects things like:

errors? && byebug

into:

errors? &&

@jonas054 jonas054 added the bug label Sep 28, 2016
@jonas054
Copy link
Collaborator

Confirmed as a bug.

@ilansh
Copy link
Contributor

ilansh commented Apr 26, 2017

@jonas054 I was thinking how to fix this issue, and came up with a few possible approaches:

  1. Cancel autocorrection for this cop.

  2. Implement autocorrection only for the simple case as it is today (standalone debugger call - without conditionals, operators etc.), issue warning for all other cases.

  3. Add autocorrection for typical debugger use cases which currently break syntax after being autocorrected (maybe I am missing some use cases?):

    • after logical operator: error? && byebug (remove entire expression, or just operator and following debugger call? )
    • under conditional modifier byebug if error? (remove entire expression)
    • within ternary conditional error? ? byebug : foo (autocorrect to conditional modifier:
      foo unless error?)

    The issue with this approch is that there is a lot of room for convoluted cases to still break the syntax or cause unexpected results, e.g.

    • error1? && cleanup && byebug do we remove entire expression in this case?
    • (x = y) == 3 if byebug what about the asignment?
    • error1? ? (error2? && byebug) : foo etc.

What do you think?

@jonas054
Copy link
Collaborator

jonas054 commented Apr 27, 2017

I recommend option 1. It's too hard to get autocorrection right for this cop, because even if you implement the conservative autocorrection according to option 2, there's still the question of comments around the removed code that may have become irrelevant or incorrect.

@Drenmi
Copy link
Collaborator

Drenmi commented Apr 27, 2017

I agree with @jonas054. I would rather have a simple cop that does 80% of the job than an overly complex one that does 90% and is prone to bugs. 🙂

@ilansh
Copy link
Contributor

ilansh commented Apr 27, 2017

Sounds good 👍

ilansh added a commit to ilansh/rubocop that referenced this issue Apr 30, 2017
It was decided to cancel auto-correction for this cop (see discussion in issue rubocop#3400)
because it is too copmlex to implement correctly, and probably not worth the effort.
@bbatsov bbatsov closed this as completed in cc04647 May 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants