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

Accept yoda condition which isn't commutative #4688

Conversation

@fujimura
Copy link
Contributor

@fujimura fujimura commented Aug 30, 2017

Regexp#=== performs matching, but String#=== is for equality. Changing position of operands will change original behavior in this case.

For example,

[1] pry(main)> "foobar" === /foo/
=> false
[2] pry(main)> /foo/ === "foobar"
=> true
@knu
Copy link
Contributor

@knu knu commented Aug 30, 2017

I guess this cop is not even aware of the === operator getting caught in the first place, as it is a subsumption operator that is obviously not generally commutative.

So, I think the best fix would be to make yoda_condition? return false when the operator is ===.

@bbatsov
Copy link
Collaborator

@bbatsov bbatsov commented Aug 30, 2017

So, I think the best fix would be to make yoda_condition? return false when the operator is ===.

Yeah, that sounds about right.

@fujimura
Copy link
Contributor Author

@fujimura fujimura commented Aug 30, 2017

Yeah, that sounds about right.

Added commit to skip === in YodaCondition. Let me know if you need squashed commit of them.

@@ -64,6 +66,8 @@ def yoda_condition?(node)
return false if non_equality_operator?(operator)
end

return false if noncommutative_operator?(operator)

This comment has been minimized.

@bbatsov

bbatsov Aug 30, 2017
Collaborator

Btw, shouldn't we actually change the definition of comparison_method? instead? //cc @Drenmi

This comment has been minimized.

@fujimura

fujimura Sep 1, 2017
Author Contributor

Excluding === from comparison_method? introduces unexpected behaviour of SafeAssignment.

I think the code below is still unsafe assignment, but if === isn't a comparison, cop can't detect it.

foo = /bar/ === 'baz'

So, my suggestion is to keep === being a comparison and skip noncommutative operation in YodaCondition cop.

This comment has been minimized.

@fujimura

fujimura Sep 1, 2017
Author Contributor

FYI: Commit to exclude === from comparision: fujimura@d9f4fd1

This comment has been minimized.

@Drenmi

Drenmi Sep 1, 2017
Collaborator

I guess the case equality operator could be a comparison method or not, depending on how one frames it. I'm not super worried about semantics here. If it's useful to remove it from the list, I say we do so. 🙂 I think it could make sense, since explicit use of #=== is already discouraged entirely in Style/CaseEquality.

This comment has been minimized.

@bbatsov

bbatsov Sep 1, 2017
Collaborator

Indeed.

@@ -86,7 +87,7 @@
)

it_behaves_like(
'autocorrect', 'false === foo ? bar : baz', 'foo === false ? bar : baz'
'autocorrect', 'false === foo ? bar : baz', 'false === foo ? bar : baz'

This comment has been minimized.

@bbatsov

bbatsov Sep 1, 2017
Collaborator

This is pointless now and should be removed.

@bbatsov
Copy link
Collaborator

@bbatsov bbatsov commented Sep 1, 2017

Please, add a changelog entry for this and squash the commits together.

Regexp#=== performs matching, but String#=== is for equality.
@fujimura fujimura force-pushed the fujimura:accept-yoda-condition-which-isn-t-commutative branch from 8282d39 to 2f47d0c Sep 4, 2017
@fujimura
Copy link
Contributor Author

@fujimura fujimura commented Sep 4, 2017

Updated.

@bbatsov bbatsov merged commit 61da69c into rubocop:master Sep 4, 2017
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bbatsov
Copy link
Collaborator

@bbatsov bbatsov commented Sep 4, 2017

👍

@fujimura fujimura deleted the fujimura:accept-yoda-condition-which-isn-t-commutative branch Sep 4, 2017
@mpotter mpotter mentioned this pull request Oct 21, 2017
5 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants