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

use-isnan options for implicit Strict Equality Comparison #12207

Assignees
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules

Comments

@mdjermanovic
Copy link
Member

What rule do you want to change?

use-isnan

This rule targets foo === NaN (Strict Equality Comparison)

(also targets !==, == and !=)

Currently, the rule checks only BinaryExpression nodes, i.e. only explicit comparisons.

This is a proposal to also check the following:

Does this change cause the rule to produce more or fewer warnings?

More if an option is set to true. Defaults are false.

How will the change be implemented? (New option, new default behavior, etc.)?

2 options, enforceForSwitchCase and enforceForIndexOf.

Please provide some example code that this change will affect:

/*eslint use-isnan: "error"*/

switch (foo) {
    case NaN:
        bar();
        break;
}

foo.indexOf(NaN);
foo.lastIndexOf(NaN);

What does the rule currently do for this code?

Nothing.

What will the rule do after it's changed?

/*eslint use-isnan: ["error", {"enforceForSwitchCase": true}]*/

switch (foo) {
    case NaN: // error
        bar();
        break;
}
/*eslint use-isnan: ["error", {"enforceForIndexOf": true}]*/

foo.indexOf(NaN); // error
foo.lastIndexOf(NaN); // error

Are you willing to submit a pull request to implement this change?

Yes. (there is PR #12106 for enforceForSwitchCase)

@mdjermanovic mdjermanovic added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Sep 3, 2019
@mdjermanovic mdjermanovic self-assigned this Sep 3, 2019
@mysticatea
Copy link
Member

Thank you for this issue.

Sounds a good idea.

@platinumazure
Copy link
Member

I'm definitely 👍 for case clauses.

I'm less sure about indexOf just due to lack of strong typing, but I won't oppose that (and I acknowledge that we have precedent with other Array.prototype functions).

@ljharb
Copy link
Sponsor Contributor

ljharb commented Sep 4, 2019

it’d be better to make a custom rule forbidding indexOf in favor of includes, for that one.

@mdjermanovic
Copy link
Member Author

I'm less sure about indexOf just due to lack of strong typing, but I won't oppose that (and I acknowledge that we have precedent with other Array.prototype functions).

The initial idea was one option for both, but exactly because of this user should have a way to disable indexOf check. I think it's still useful in most cases, but has to be always behind an option (unlike case clauses, which could be default behavior in next major version).

@mdjermanovic
Copy link
Member Author

it’d be better to make a custom rule forbidding indexOf in favor of includes, for that one.

This can be easily done with no-restricted-properties. Or probably with no-restricted-syntax to disable just === -1, >= 0 etc. for which includes is equvalent.

@g-plane
Copy link
Member

g-plane commented Sep 9, 2019

Off topic:
eslint-plugin-unicorn has a rule to advise to use Array.prototype.includes.

@mysticatea mysticatea added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Sep 9, 2019
@mdjermanovic
Copy link
Member Author

I'm working on this (indexOf).

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Apr 24, 2020
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Apr 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.