Fix: yoda left string fix for exceptRange (fixes #12883)

[anikethsaha]

Prerequisites checklist

• I have read the contributing guidelines.
• The team has reached consensus on the changes proposed in this pull request. If not, I understand that the evaluation process will begin with this pull request and won't be merged until the team has reached consensus.

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[X] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

fixing the second issue in #12883

What changes did you make? (Give an overview)

as mentioned in #12883, when having a string in exceptRange in left, it was throwing error. So, I added a string check in isBetwwenTest method for leftLiteral.value

Is there anything you'd like reviewers to focus on?

fixes #12883

[anikethsaha]

@mdjermanovic I would like to apologize as I forgot to comment on the issue that I am picking this! my bad !

[mdjermanovic]

No problem, thanks for the PR!

[yeonjuan]

@anikethsaha
Thanks for the PR. I didn't check the code part thoroughly, but I could find some false positive in test cases. I commented at one of them.

[anikethsaha]

@yeonjuan can you point those false positive. It would be better for me to solve them

[anikethsaha]

will it help for reviewing if I comment along with the invalid cases with why they are invalid?

[yeonjuan]

@anikethsaha

can you point those false positive. It would be better for me to solve them

Yes :)
Some of the cases which use the operator || with the option expectRange: true seem like false positive. The blow cases are not warned in the actual version(master) but warned in this PR.

• Online Demo

/*eslint yoda: ["error", "never", { "exceptRange": true }]*/
if (x < -1n || 1n <= x) {}
if (x < -1 || 1 <= x) {}

In my understanding, those cases should not be warned because they using the option expectRange: true and they check about ranges.

Examples of correct code for the "never", { "exceptRange": true } options:
/eslint yoda: ["error", "never", { "exceptRange": true }]/
// ...
if (x < -1 || 1 < x) {
// ...
}

And, we can find about the proper behavior of the option exceptRange: true in the docs.
If I missed something please tell me. Thanks 😄

[anikethsaha]

@yeonjuan , thanks for pointing.

(x < -1n || 1n <= x)

I think the docs is wrong about this, cause this is not a yoda condition, right ! , so with never options (which allows non-yoda only) it will be lint-free but when using exceptRange it will only allow yoda for range condition, so this why it is invalid

let me know if I am wrong about this. !

[anikethsaha]

@mdjermanovic @yeonjuan I have added a comment for why some code should be invalid which is currently showing valid (in master), do let me know if anything else is required.

[mdjermanovic]

(x < -1n || 1n <= x)

I think the docs is wrong about this, cause this is not a yoda condition, right ! , so with never options (which allows non-yoda only) it will be lint-free but when using exceptRange it will only allow yoda for range condition, so this why it is invalid.

1n <= x is a yoda condition. This would be an error with "never", { "exceptRange": false }.

This isn't an error with "never", { "exceptRange": true } because it's used in a valid range test.

[yeonjuan]

@anikethsaha
Reply for the comment 😄.

I think the docs is wrong about this,

I don't think so. I think the documentation is right.

if (x < -1n || 1n <= x) {}

cause this is not a yoda condition

Yes, this is not a yoda condition but its range test is valid. so when the option exceptRange is set to true it should not be warned.

so with never options (which allows non-yoda only) it will be lint-free

No. I don't think so. As you mentioned it enforce non-yoda with the never option. but there is a yoda condition (1n < x) on the right-side so it would be warned if the exceptRange: false

[anikethsaha]

let me clarify if I get this rule correctly !

• when , the range is in yoda, eg (x < -1n || 1n <= x), - yoda : 1n <= x

  • with never , it should be an error
  • with never and exceptRange : true , it wont be an error right ? cause it expects the range to be in yoda
  • with always, it should not be an error
  • with always and exceptRange : true , it will be an error as it expects the condition to be non - yoda

• when the range is not in yoda , eg (x <= -1n || x > 1n)

  • with never, it is lint free
  • with never and exceptRange : true , it is error as it expects the range condition to be in yoda
  • with always, it is error
  • with always and exceptRange : true, it should not be an error as it expects the range condition to be non-yoda

So, from above, we can see that range condition is only allowed for

• [ 'never', { exceptRange : true } ]
• [ 'always']

After these checks, the rule can still show a linting error if the condition does not returns true

like this

input	left	right	checking	result	rule options
(1 < a && a <= 2)	1	2	1 <= 2	true	never, exceptRange : true
('bar' < a && a <= 'foo')	bar	foo	'bar' <= 'foo'	true	never, exceptRange : true
(2 < a && a <= 1)	2	1	2 <= 1	false	never , exceptRange : true
(x < -1 !! 1 < x)	-1	1	-1<1	true	never, exceptRange : true

Please let me know if any of this wrong !

cc @yeonjuan @mdjermanovic

Please check the latest commit, I think those false negatives are fixed

[mdjermanovic]

Thanks for the changes! This looks good, just needs some small modifications.

It seems that this PR will fix both 1. and 2. from #12883 👍

[yeonjuan]

Thanks for keep working on! 👍 In addition to other's reviews, I left a comment.

[jsf-clabot]

All committers have signed the CLA.

[anikethsaha]

test for yoda are passing but not sure why tests for other rules (prefer-regex-literals, prefer-exponentiation-operator etc) is failing

[anikethsaha]

test for yoda are passing but not sure why tests for other rules (prefer-regex-literals, prefer-exponentiation-operator etc) is failing

cc @mdjermanovic @yeonjuan any suggestion for fixing this? I think its not related to this changes.

[mdjermanovic]

test for yoda are passing but not sure why tests for other rules (prefer-regex-literals, prefer-exponentiation-operator etc) is failing

cc @mdjermanovic @yeonjuan any suggestion for fixing this? I think its not related to this changes.

It looks like you don't have the new version of eslint-utils installed.

CI is failing for a different reason (#13076)

[anikethsaha]

cc @mdjermanovic I was behind upstream master ! CI is clean

[yeonjuan]

Thanks for changing. 👍 I left some minor suggestions.

[yeonjuan]

LGTM! Thanks

[mdjermanovic]

Looks great, thanks!

[kaicataldo]

LGTM, thank you!