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

autocorrect creates invalid code on 'or' replacement #387

Closed
dirkbolte opened this issue Jul 23, 2013 · 7 comments · Fixed by #415
Closed

autocorrect creates invalid code on 'or' replacement #387

dirkbolte opened this issue Jul 23, 2013 · 7 comments · Fixed by #415
Assignees
Labels

Comments

@dirkbolte
Copy link
Contributor

With auto-correct, the following code will be corrected into an invalid syntax:

teststring = 'abcdefg'
puts "#{teststring.include? 'a' or teststring.include? 'b'}"

becomes

teststring = 'abcdefg'
puts "#{teststring.include? 'a' || teststring.include? 'b'}"

A possible correction would be:

teststring = 'abcdefg'
puts "#{teststring.include? 'a' || teststring.include?('b')}"
@dirkbolte
Copy link
Contributor Author

rubocop version is: 0.10.0

@bbatsov
Copy link
Collaborator

bbatsov commented Jul 24, 2013

That's a known problem. Doing the autocorrection correctly is pretty complex, so we took a sneaky shortcut here. Maybe we should drop this particular autocorrection? Or we could simply correct only nodes for which replacing or with || won't break anything.

Maybe @jonas054, @edzhelyov and @yujinakayama have a better solution in mind?

@jonas054
Copy link
Collaborator

No I don't have a better solution. We should do as you suggest. But I have a feeling it could be complicated to get it right. In that case we should remove this autocorrection first, and release, then implement an autocorrection of or to || that always works.

@yujinakayama
Copy link
Collaborator

For now, I also think we don't have better way.

In the future, I think probably we need to implement some way to handle precedences of Ruby syntax, though I guess it's not easy and requires robust logic.

@edzhelyov
Copy link
Collaborator

Currently we have only 3-4 simple auto corrections and I expect to have similar issues.
I propose that we mark the auto-correct option as experimental and try to improve it.
We may try to run the parser again over the corrected files and abort the correction if there are syntax problems.

@bbatsov
Copy link
Collaborator

bbatsov commented Jul 27, 2013

@edzhelyov Makes a good point.

@ghost ghost assigned jonas054 Aug 3, 2013
@jonas054
Copy link
Collaborator

jonas054 commented Aug 3, 2013

I'll try to implement something based on what @edzhelyov said.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants