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

lib: introduce no-mixed-operators eslint rule to lib #29834

Closed
wants to merge 2 commits into from

Conversation

ZYSzys
Copy link
Member

@ZYSzys ZYSzys commented Oct 3, 2019

Introduce no-mixed-operators eslint rule to lib for better code readability.

Refs: #29825 (comment)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Oct 3, 2019
@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 3, 2019
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with my comments addressed.

lib/readline.js Outdated Show resolved Hide resolved
val1.size === 0)) {
((iterationType === kNoIterator ||
iterationType === kIsArray) && (val1.length === 0 ||
val1.size === 0))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be a typo. The condition does not seem to be identical to the original one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@ZYSzys ZYSzys Oct 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Trott removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 3, 2019
Trott
Trott previously requested changes Oct 3, 2019
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

condition change needs to be sorted out

@Trott Trott dismissed their stale review October 4, 2019 02:07

code has changed

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Trott pushed a commit that referenced this pull request Oct 5, 2019
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>
@Trott
Copy link
Member

Trott commented Oct 5, 2019

Landed in 739f113

@Trott Trott closed this Oct 5, 2019
BridgeAR pushed a commit that referenced this pull request Oct 9, 2019
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>
@BridgeAR BridgeAR mentioned this pull request Oct 10, 2019
@ZYSzys ZYSzys deleted the eslint-no-mixed-operators branch October 20, 2019 05:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants