lib: introduce no-mixed-operators eslint rule to lib#29834
lib: introduce no-mixed-operators eslint rule to lib#29834ZYSzys wants to merge 2 commits intonodejs:masterfrom zys-contrib:eslint-no-mixed-operators
Conversation
nodejs-github-bot
added
the
lib / src
Trott
added
the
author ready
BridgeAR
left a comment
BridgeAR
left a comment
There was a problem hiding this comment.
LGTM with my comments addressed.
lib/internal/util/comparisons.js
Outdated
| val1.size === 0)) { | ||
| ((iterationType === kNoIterator || | ||
| iterationType === kIsArray) && (val1.length === 0 || | ||
| val1.size === 0))) { |
There was a problem hiding this comment.
This seems to be a typo. The condition does not seem to be identical to the original one.
There was a problem hiding this comment.
Oh, yes, this is different. For example, if aKeys.length === 0 and iterationType === kNoIterator are both true but the remaining three conditions are all false, the current code evaluates to true but the modified code evaluates to false. Might be good to add a test for this if possible, since all tests pass with this change. If it's not possible to add a test, it may indicate that this conditional can be simplified.
There was a problem hiding this comment.
Good catch.
So here I found that always && should be within parenthesis in every mixed operators && and ||.
Might be good to add a test for this if possible, since all tests pass with this change. If it's not possible to add a test, it may indicate that this conditional can be simplified.
@Trott It seems a little hard to add tests here for me. Is there a user case could actually catch this ?
And if this condition need to be simplified, maybe it's better to do it in another PR.
There was a problem hiding this comment.
This is a fast path. If it returns false when it should return true, it will still work. Testing this is almost impossible (only the timing could be tested for plus the coverage).
Trott
removed
the
author ready
Trott
left a comment
Trott
left a comment
There was a problem hiding this comment.
condition change needs to be sorted out
PR-URL: #29834 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
|
Landed in 739f113 |
PR-URL: #29834 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
10 participants
Introduce
no-mixed-operatorseslint rule tolibfor better code readability.Refs: #29825 (comment)
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes