feat: add suggestions to `use-isnan` in binary expressions #17996

StyleShit · 2024-01-16T18:57:46Z

Partially handles #17978
This PR adds support for fix suggestions in use-isnan only in binary expressions, so this:

x === NaN;
y !== NaN;

will be fixed to this:

Number.isNaN(x);
! Number.isNaN(y);

linux-foundation-easycla · 2024-01-16T18:57:50Z

The committers listed above are authorized under a signed CLA.

✅ login: StyleShit / name: StyleShit (e2057a4, 8f9d277, 892cae9, bf1965d, f735887, 098c6d0, 9d02fbe, d844e03, 24c4238, 7266bad, e98e980, a64c253, 0481f3d)

netlify · 2024-01-16T18:57:51Z

✅ Deploy Preview for docs-eslint canceled.

Name	Link
🔨 Latest commit	`0481f3d`
🔍 Latest deploy log	https://app.netlify.com/sites/docs-eslint/deploys/65b11f377e2e070008601fd4

StyleShit · 2024-01-16T18:58:55Z

tests/lib/rules/use-isnan.js

+        {
+            code: `/* just
+                adding */ x /* some */ === /* comments */ NaN; // here`,
+            errors: [{
+                ...comparisonError,
+                suggestions: [{
+                    messageId: "replaceWithIsNaN",
+                    output: `/* just
+                adding */ Number.isNaN(x); // here`
+                }]
+            }]


I wasn't sure what to do with the comments here, so I left the default behavior for now.
Any direction on what's the expected behavior here?

I think this looks okay to me.

Is it OK to lose some comments when fixing?

Good question -- ideally we don't lose any comments, but since this is a suggestion, I think it's okay because people will manually need to accept it and can always undo it if it's a problem.

I've had a similar case in ts-eslint, and there we've decided to aggregate all of the comments and throw them somewhere above the code.

Code:
https://github.com/typescript-eslint/typescript-eslint/blob/01556f50376b87eff21c42d121b627e3a6f003f0/packages/eslint-plugin/src/rules/no-array-delete.ts#L89-L102

Test:
https://github.com/typescript-eslint/typescript-eslint/blob/01556f50376b87eff21c42d121b627e3a6f003f0/packages/eslint-plugin/tests/rules/no-array-delete.test.ts#L510-L547

Do you think it's worth doing the same here?

I also think it's okay for suggestions to just remove comments that happen to be there. As @nzakas noted, if it's a problem in some cases, user can undo the change and manually fix the code.

nzakas

Thanks for this. The functionality for the equality operators looks good.

We shouldn't supply suggestions for comparison operators like < and >. Because this isn't an equality operation, we can't use isNaN() as a replacement for those.

You say this partially handles #17978 but you didn't indicate what part you are addressing and what part(s) you're not. Can you please update the description of the PR to explain what specifically you are addressing from the issue?

nzakas · 2024-01-16T20:32:06Z

tests/lib/rules/use-isnan.js

+        {
+            code: `/* just
+                adding */ x /* some */ === /* comments */ NaN; // here`,
+            errors: [{
+                ...comparisonError,
+                suggestions: [{
+                    messageId: "replaceWithIsNaN",
+                    output: `/* just
+                adding */ Number.isNaN(x); // here`
+                }]
+            }]


I think this looks okay to me.

…tions-binary

StyleShit · 2024-01-17T17:08:57Z

We shouldn't supply suggestions for comparison operators like < and >.

Done

update the description of the PR

Yeah, sure, sorry for that. Updated 😄

lib/rules/use-isnan.js

…tions-binary

nzakas

LGTM. Would like another approval before merging.

lib/rules/use-isnan.js

mdjermanovic · 2024-01-19T20:40:42Z

foo == NaN looks like intent to convert the other operand to a number, so it might make more sense to suggest isNaN(foo) because, unlike Number.isNaN(), it converts the given value to a number, or maybe Number.IsNaN(Number(foo)) if we want to avoid isNaN()?

const foo = "bar";

Number.isNaN(foo); // false
isNaN(foo); // true
Number.isNaN(Number(foo)); // true

The same for foo != NaN.

lib/rules/use-isnan.js

StyleShit · 2024-01-21T16:05:59Z

foo == NaN looks like intent to convert the other operand to a number, so it might make more sense to suggest isNaN(foo) because, unlike Number.isNaN(), it converts the given value to a number, or maybe Number.IsNaN(Number(foo)) if we want to avoid isNaN()?
const foo = "bar";

Number.isNaN(foo); // false
isNaN(foo); // true
Number.isNaN(Number(foo)); // true
The same for foo != NaN.

I'm not sure we can assume the user intent (Well... yeah, we basically assume the intent by giving those suggestions, so... 😅), but if I understand correctly, this makes sense.

Not sure though... Maybe we can suggest both and leave the choice to the user? WDYT?

mdjermanovic · 2024-01-21T16:31:52Z

Maybe we can suggest both and leave the choice to the user?

By both, you mean Number.IsNaN(foo) as one suggestion, and one of isNaN(foo)/Number.isNaN(Number(foo)) as another suggestion?

StyleShit · 2024-01-21T16:40:23Z

Maybe we can suggest both and leave the choice to the user?

By both, you mean Number.IsNaN(foo) as one suggestion, and one of isNaN(foo)/Number.isNaN(Number(foo)) as another suggestion?

Yup, exactly

nzakas · 2024-01-22T18:10:20Z

I think it's fine to leave two suggestions, one for each, though I would put Number.isNaN() first.

…tions-binary

StyleShit · 2024-01-23T18:38:30Z

Added 2 suggestions support.
I think it's kinda weird that we cast things like 123 to Number.
Is there any way to avoid that or we must have types for this?

mdjermanovic · 2024-01-24T14:00:31Z

tests/lib/rules/use-isnan.js

        {
            code: "123 === NaN;",
-            errors: [comparisonError]
+            errors: [{
+                ...comparisonError,
+                suggestions: [
+                    {
+                        messageId: "replaceWithIsNaN",
+                        output: "Number.isNaN(123);"
+                    },
+                    {
+                        messageId: "replaceWithCastingAndIsNaN",
+                        output: "Number.isNaN(Number(123));"
+                    }
+                ]
+            }]
        },


For === or !==, I think there should be only one suggestion (without Number()).

Ohh, yeah, this makes more sense. Now I get what you mean.
Done :)

mdjermanovic · 2024-01-24T14:05:58Z

I think it's kinda weird that we cast things like 123 to Number.

I think it's fine because a code like 123 === NaN doesn't make sense anyway.

mdjermanovic

LGTM, thanks! Leaving open for @nzakas to re-verify.

nzakas

LGTM. Nice work!

nzakas · 2024-02-08T23:28:05Z

@StyleShit we'd like to pay you for this contribution, but I couldn't find an email address to send you the details. Can you please contact us at contact (at) eslint (dot) org? Thanks.

feat: add suggestions to use-isnan in binary expressions

e2057a4

eslint-github-bot bot added the feature This change adds a new feature to ESLint label Jan 16, 2024

StyleShit commented Jan 16, 2024

View reviewed changes

StyleShit marked this pull request as ready for review January 16, 2024 19:08

StyleShit requested a review from a team as a code owner January 16, 2024 19:08

nzakas requested changes Jan 16, 2024

View reviewed changes

Tanujkanti4441 added rule Relates to ESLint's core rules evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Jan 17, 2024

StyleShit added 2 commits January 17, 2024 19:08

remove > < from fixes

8f9d277

Merge remote-tracking branch 'eslint/main' into feat/use-isnan-sugges…

892cae9

…tions-binary

StyleShit requested a review from nzakas January 17, 2024 17:09

nzakas requested changes Jan 17, 2024

View reviewed changes

lib/rules/use-isnan.js Outdated Show resolved Hide resolved

StyleShit requested a review from nzakas January 18, 2024 06:09

StyleShit added 3 commits January 18, 2024 08:10

move the array up

bf1965d

use Set

f735887

Merge remote-tracking branch 'eslint/main' into feat/use-isnan-sugges…

098c6d0

…tions-binary

nzakas added accepted There is consensus among the team that this change meets the criteria for inclusion contributor pool labels Jan 19, 2024

nzakas approved these changes Jan 19, 2024

View reviewed changes

mdjermanovic removed the evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion label Jan 19, 2024

mdjermanovic reviewed Jan 19, 2024

View reviewed changes

lib/rules/use-isnan.js Outdated Show resolved Hide resolved

mdjermanovic reviewed Jan 19, 2024

View reviewed changes

lib/rules/use-isnan.js Outdated Show resolved Hide resolved

StyleShit added 3 commits January 21, 2024 18:12

wip

9d02fbe

small refactor

d844e03

remove empty line

24c4238

StyleShit requested a review from mdjermanovic January 21, 2024 16:15

StyleShit added 3 commits January 23, 2024 20:34

add multiple suggestions

7266bad

Merge remote-tracking branch 'eslint/main' into feat/use-isnan-sugges…

e98e980

…tions-binary

change wording

a64c253

StyleShit requested a review from nzakas January 23, 2024 18:38

mdjermanovic reviewed Jan 24, 2024

View reviewed changes

suggest casting only in == and !=

0481f3d

StyleShit requested a review from mdjermanovic January 24, 2024 14:34

mdjermanovic approved these changes Jan 25, 2024

View reviewed changes

nzakas approved these changes Jan 29, 2024

View reviewed changes

nzakas merged commit 2d11d46 into eslint:main Jan 29, 2024
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add suggestions to `use-isnan` in binary expressions #17996

feat: add suggestions to `use-isnan` in binary expressions #17996

StyleShit commented Jan 16, 2024 •

edited

linux-foundation-easycla bot commented Jan 16, 2024 •

edited

netlify bot commented Jan 16, 2024 •

edited

StyleShit Jan 16, 2024

nzakas Jan 16, 2024

StyleShit Jan 17, 2024

nzakas Jan 17, 2024

StyleShit Jan 18, 2024

mdjermanovic Jan 19, 2024

nzakas left a comment

nzakas Jan 16, 2024

StyleShit commented Jan 17, 2024

nzakas left a comment

mdjermanovic commented Jan 19, 2024

StyleShit commented Jan 21, 2024

mdjermanovic commented Jan 21, 2024

StyleShit commented Jan 21, 2024

nzakas commented Jan 22, 2024

StyleShit commented Jan 23, 2024 •

edited

mdjermanovic Jan 24, 2024

StyleShit Jan 24, 2024 •

edited

mdjermanovic commented Jan 24, 2024

mdjermanovic left a comment

nzakas left a comment

nzakas commented Feb 8, 2024

feat: add suggestions to use-isnan in binary expressions #17996

feat: add suggestions to use-isnan in binary expressions #17996

Conversation

StyleShit commented Jan 16, 2024 • edited

linux-foundation-easycla bot commented Jan 16, 2024 • edited

netlify bot commented Jan 16, 2024 • edited

✅ Deploy Preview for docs-eslint canceled.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nzakas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StyleShit commented Jan 17, 2024

nzakas left a comment

Choose a reason for hiding this comment

mdjermanovic commented Jan 19, 2024

StyleShit commented Jan 21, 2024

mdjermanovic commented Jan 21, 2024

StyleShit commented Jan 21, 2024

nzakas commented Jan 22, 2024

StyleShit commented Jan 23, 2024 • edited

Choose a reason for hiding this comment

StyleShit Jan 24, 2024 • edited

Choose a reason for hiding this comment

mdjermanovic commented Jan 24, 2024

mdjermanovic left a comment

Choose a reason for hiding this comment

nzakas left a comment

Choose a reason for hiding this comment

nzakas commented Feb 8, 2024

feat: add suggestions to `use-isnan` in binary expressions #17996

feat: add suggestions to `use-isnan` in binary expressions #17996

StyleShit commented Jan 16, 2024 •

edited

linux-foundation-easycla bot commented Jan 16, 2024 •

edited

netlify bot commented Jan 16, 2024 •

edited

StyleShit commented Jan 23, 2024 •

edited

StyleShit Jan 24, 2024 •

edited