lib: introduce no-mixed-operators eslint rule to lib #29834
Conversation
nodejs-github-bot
added
the
lib / src
Trott
added
the
author ready
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
PR-URL: #29834 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
|
Landed in 739f113 |
PR-URL: #29834 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
Introduce
no-mixed-operatorseslint rule tolibfor better code readability.Refs: #29825 (comment)
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes