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

auto-correct changes logic #2859

Closed
xithan opened this issue Feb 17, 2016 · 6 comments
Closed

auto-correct changes logic #2859

xithan opened this issue Feb 17, 2016 · 6 comments

Comments

@xithan
Copy link

xithan commented Feb 17, 2016

a == a.downcase is changed to a.casecmp(a).zero?
(the former condition is supposed to check if uppercase letters are present; the auto-corrected condition is always true)

return if (!a.include? b) && c is changed to return unless a.include? b && c
(auto-correct converts if ! to unless although there is a second expression involved)

@rrosenblum
Copy link
Contributor

@xithan, the first issue that you mention, pertaining to Perfomance/Casecmp has a more detailed discussion in #2852. Please join the conversation there.

The second one sounds like a bug to me. I assume that the second issue is coming from Style/NegatedIf in combination with Style/ParenthesesAroundCondition or more likely Style/RedundantParentheses. Can you please provide the version of RuboCop that you are using as well as whether or not the listed cops are enabled or disabled.

@xithan
Copy link
Author

xithan commented Feb 19, 2016

latest rubocop version 0.37.2 with default style rules. I double-checked with rubocop --show-cops:
Style/NegatedIf enabled
Style/RedundantParentheses enabled
Style/ParenthesesAroundCondition enabled

ParenthesesAroundCondition doesn't matter. With NegatedIf enabled, the other two disabled and removed parentheses return if !a.include? b && c is still changed to return unless a.include? b && c

@rrosenblum
Copy link
Contributor

Running RuboCop on the code produces this output:

test.rb:4:1: C: [Corrected] Style/NegatedIf: Favor unless over if for negative conditions.
return if !a.include? b && c
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
test.rb:4:11: C: [Corrected] Style/RedundantParentheses: Don't use parentheses around an unary operation.
return if (!a.include? b) && c
          ^^^^^^^^^^^^^^^
test.rb:4:15: W: Lint/RequireParentheses: Use parentheses in the method call to avoid confusion about precedence.
return unless a.include? b && c

Breaking it down, the first run will cause an offense in Style/RedundantParentheses. The second run will cause a new offense in Style/NegatedIf.

I think this is an issue with Style/NegatedIf because return if (!a.include?) b && c and return if !a.include? b && c should be equivalent code, but return unless a.include? b && c is not.

@alexdowad
Copy link
Contributor

I think this is an issue with Style/NegatedIf because return if (!a.include?) b && c and return if !a.include? b && c should be equivalent code, but return unless a.include? b && c is not.

Not so. The transformation which NegatedIf is performing is valid. It doesn't look valid, but it is (precedence is a bit deceiving here).

RedundantParentheses is the problem.

@alexdowad
Copy link
Contributor

Just pushing a fix to my open PR.

@rrosenblum
Copy link
Contributor

Thanks for the fix @alexdowad

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

No branches or pull requests

3 participants