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

Merged
merged 1 commit into from Sep 4, 2017

Conversation

Projects
None yet
4 participants
@fujimura
Copy link
Contributor

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

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Collaborator

bbatsov commented Sep 1, 2017

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

Accept yoda condition which isn't commutative
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

This comment has been minimized.

Copy link
Contributor Author

fujimura commented Sep 4, 2017

Updated.

@bbatsov bbatsov merged commit 61da69c into rubocop-hq:master Sep 4, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bbatsov

This comment has been minimized.

Copy link
Collaborator

bbatsov commented Sep 4, 2017

👍

@fujimura fujimura deleted the fujimura:accept-yoda-condition-which-isn-t-commutative branch Sep 4, 2017

@mpotter mpotter referenced this pull request Oct 21, 2017

Merged

Upgrade to rubocop 0.51.0 #55

5 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.