tools: add number-isnan eslint rule #17556

maclover7 · 2017-12-08T21:14:09Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

tools, test

apapirovski · 2017-12-08T21:46:41Z

This might be a bit finicky since it would catch const isNaN = Number.isNaN; isNaN(val); too? But maybe that's not a real problem... 😆

Trott · 2017-12-08T21:54:06Z

No objections, but one question and one observation.

Question: What's the reason for avoiding the isNaN() global?

Observation: I think this can probably be implemented using no-restricted-syntax rather than a custom rule.

maclover7 · 2017-12-09T21:19:48Z

@Trott

Question: What's the reason for avoiding the isNaN() global?

For personal reasons, I think it's more clear to use Number.isNaN. Also, several Code and Learn PRs have been submitted to move towards Number.isNaN, so I figured that people had come to consensus that it was a good idea.

Observation: I think this can probably be implemented using no-restricted-syntax rather than a custom rule.

Interesting -- refactored down to use no-restricted-syntax

apapirovski

Unfortunately this won't work as it overrides all the no-restricted-syntax rules declared within .eslintrc.yaml in the root directory.

Trott · 2017-12-10T03:45:47Z

Unfortunately this won't work as it overrides all the no-restricted-syntax rules declared within .eslintrc.yaml in the root directory.

Oh, yeah, I assumed it was a lint rule we'd want for the entire code base...

apapirovski · 2017-12-10T03:47:04Z

Oh, yeah, I assumed it was a lint rule we'd want for the entire code base...

Yeah, I think we could do that? I don't see any reason why we would be stricter about this in test than lib.

Trott · 2017-12-10T03:48:49Z

Are we sure we want to necessarily encourage people to change isNaN() to Number.isNan(). They behave differently.

isNaN('a string is not a nubmer'); //true
Number.isNaN('a string is not a number'); // false

apapirovski · 2017-12-10T03:52:39Z

Number.isNaN should be the new standard way of doing this as per ES2015. There's weird coercion in isNaN.

maclover7 · 2017-12-11T22:22:23Z

updated @apapirovski @Trott

For context, I was initially hesitant to add the rule to lib/, since we are generally performance sensitive, but I don't think the change will massively alter performance. Should benchmarks be run anyways?

Trott · 2017-12-11T22:33:05Z

@maclover7 Sorry to make this tedious, but this is what I'd recommend:

Revert to your initial custom rule and apply it just to test
If you wish, in a separate PR, propose changing isNaN() to Number.isNaN() in lib and prepare to run benchmarks and have a fair amount of conversation about it. :-D
If the above bullet point suggestion lands, then use the restricted-syntax thing to replace your custom rule.

Trott · 2017-12-11T22:33:33Z

(And yeah, had I not said anything, first bullet point would have happened and all of this would have been avoided. Sorry!)

not-an-aardvark · 2017-12-13T19:14:23Z

lib/events.js

@@ -75,7 +75,7 @@ EventEmitter.init = function() {
 // Obviously not all Emitters should be limited to 10. This function allows
 // that to be increased. Set to zero for unlimited.
 EventEmitter.prototype.setMaxListeners = function setMaxListeners(n) {
-  if (typeof n !== 'number' || n < 0 || isNaN(n)) {
+  if (typeof n !== 'number' || n < 0 || Number.isNaN(n)) {


I think the typeof n !== 'number' check is redundant if Number.isNaN is being used.

not-an-aardvark · 2017-12-13T19:14:35Z

lib/readline.js

@@ -105,7 +105,7 @@ function Interface(input, output, completer, terminal) {
  }

  if (typeof historySize !== 'number' ||
-      isNaN(historySize) ||
+      Number.isNaN(historySize) ||


I think the typeof historySize !== 'number' check is redundant if Number.isNaN is being used.

maclover7 · 2017-12-14T21:54:46Z

updated @Trott, PTAL

maclover7 · 2017-12-18T22:00:19Z

CI: https://ci.nodejs.org/job/node-test-pull-request/12184/

maclover7 · 2017-12-19T00:00:36Z

Will land tomorrow unless any objections

maclover7 · 2017-12-19T18:22:10Z

Landed in e554bc8

PR-URL: nodejs#17556 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

PR-URL: #17556 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Dec 8, 2017

maclover7 force-pushed the jm-number-isnan branch from 8ebfc4a to 3397250 Compare December 8, 2017 21:16

maclover7 force-pushed the jm-number-isnan branch from 3397250 to 960dd50 Compare December 9, 2017 21:19

apapirovski reviewed Dec 9, 2017

View reviewed changes

maclover7 force-pushed the jm-number-isnan branch from 960dd50 to 1e94820 Compare December 11, 2017 22:21

not-an-aardvark reviewed Dec 13, 2017

View reviewed changes

maclover7 added 2 commits December 14, 2017 16:53

tools: add number-isnan rule

4d7d595

tools: switch to explicit number-isnan rule

301cc7a

maclover7 force-pushed the jm-number-isnan branch from b57c942 to 301cc7a Compare December 14, 2017 21:54

Trott approved these changes Dec 14, 2017

View reviewed changes

cjihrig approved these changes Dec 14, 2017

View reviewed changes

maclover7 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 19, 2017

maclover7 self-assigned this Dec 19, 2017

maclover7 closed this Dec 19, 2017

maclover7 deleted the jm-number-isnan branch December 19, 2017 18:22

maclover7 added a commit to maclover7/node that referenced this pull request Dec 19, 2017

tools: add number-isnan rule

e554bc8

PR-URL: nodejs#17556 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 29, 2017

MylesBorins pushed a commit that referenced this pull request Jan 8, 2018

tools: add number-isnan rule

e7a99b3

PR-URL: #17556 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

MylesBorins pushed a commit that referenced this pull request Jan 9, 2018

tools: add number-isnan rule

6531401

PR-URL: #17556 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

MylesBorins mentioned this pull request Jan 10, 2018

v9.4.0 proposal #18069

Merged

MylesBorins added lts-watch-v6.x labels Jan 23, 2018

MylesBorins pushed a commit that referenced this pull request Jan 23, 2018

tools: add number-isnan rule

a84d145

PR-URL: #17556 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

gibfahn pushed a commit that referenced this pull request Jan 24, 2018

tools: add number-isnan rule

0e37054

PR-URL: #17556 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

MylesBorins pushed a commit that referenced this pull request Feb 27, 2018

tools: add number-isnan rule

4879038

PR-URL: #17556 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

MylesBorins added land-on-v6.x and removed lts-watch-v6.x labels Feb 27, 2018

This was referenced Feb 27, 2018

v6.13.1 proposal #19025

Closed

v6.13.1 proposal #19027

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tools: add number-isnan eslint rule #17556

tools: add number-isnan eslint rule #17556

maclover7 commented Dec 8, 2017 •

edited

Loading

apapirovski commented Dec 8, 2017

Trott commented Dec 8, 2017

maclover7 commented Dec 9, 2017

apapirovski left a comment

Trott commented Dec 10, 2017

apapirovski commented Dec 10, 2017

Trott commented Dec 10, 2017

apapirovski commented Dec 10, 2017

maclover7 commented Dec 11, 2017

Trott commented Dec 11, 2017

Trott commented Dec 11, 2017

not-an-aardvark Dec 13, 2017

not-an-aardvark Dec 13, 2017

maclover7 commented Dec 14, 2017

maclover7 commented Dec 18, 2017

maclover7 commented Dec 19, 2017

maclover7 commented Dec 19, 2017

tools: add number-isnan eslint rule #17556

tools: add number-isnan eslint rule #17556

Conversation

maclover7 commented Dec 8, 2017 • edited Loading

Checklist

Affected core subsystem(s)

apapirovski commented Dec 8, 2017

Trott commented Dec 8, 2017

maclover7 commented Dec 9, 2017

apapirovski left a comment

Choose a reason for hiding this comment

Trott commented Dec 10, 2017

apapirovski commented Dec 10, 2017

Trott commented Dec 10, 2017

apapirovski commented Dec 10, 2017

maclover7 commented Dec 11, 2017

Trott commented Dec 11, 2017

Trott commented Dec 11, 2017

not-an-aardvark Dec 13, 2017

Choose a reason for hiding this comment

not-an-aardvark Dec 13, 2017

Choose a reason for hiding this comment

maclover7 commented Dec 14, 2017

maclover7 commented Dec 18, 2017

maclover7 commented Dec 19, 2017

maclover7 commented Dec 19, 2017

maclover7 commented Dec 8, 2017 •

edited

Loading