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

feat: add suggestions to use-isnan in indexOf & lastIndexOf calls #18063

Merged
merged 12 commits into from Feb 14, 2024

Conversation

StyleShit
Copy link
Contributor

@StyleShit StyleShit commented Feb 1, 2024

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[X] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

This PR adds suggestions to use-isnan when using indexOf and lastIndexOf.
The current fix changes them to findIndex & findLastIndex respectively.
I wanted to suggest also includes and some as second and third suggestions, but I got into a rabbit hole of checking whether the parent node is a BinaryExpression and whether the developer used indexOf() !== -1, indexOf() > 0, etc.
We can probably do that, just need to find all the cases. Do you think it's worth the effort?

(Probably?) Closes #17978

NOTE

Let's wait until #18059 is merged before merging this one, because it'll require me to extend the new tests that were added there.

@StyleShit StyleShit requested a review from a team as a code owner February 1, 2024 05:59
@eslint-github-bot eslint-github-bot bot added the feature This change adds a new feature to ESLint label Feb 1, 2024
Copy link

netlify bot commented Feb 1, 2024

Deploy Preview for docs-eslint ready!

Name Link
🔨 Latest commit ff77c61
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/65ccd080535cfa000836750c
😎 Deploy Preview https://deploy-preview-18063--docs-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@amareshsm amareshsm added the rule Relates to ESLint's core rules label Feb 1, 2024
lib/rules/use-isnan.js Outdated Show resolved Hide resolved
@nzakas
Copy link
Member

nzakas commented Feb 2, 2024

Marking as a draft so we don't accidentally merge.

@nzakas nzakas marked this pull request as draft February 2, 2024 19:03
@nzakas
Copy link
Member

nzakas commented Feb 2, 2024

I don't think it's worth the extra effort to give more than one suggestion for this. NaN checking tends to be a corner case, so I don't think it's worth going down the rabbit hole of figuring out all the ways to make code that checks for it correctly.

@StyleShit
Copy link
Contributor Author

@nzakas Ready for review 😄

@StyleShit StyleShit marked this pull request as ready for review February 8, 2024 20:16
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

Overall LGTM, just a question around naming.

* To retain side effects, it's essential to address `NaN` beforehand, which
* is not possible with fixes like `arr.findIndex(Number.isNaN)`.
*/
const isFixable = node.arguments[0].type !== "SequenceExpression";
Copy link
Member

Choose a reason for hiding this comment

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

I think isFixable actually means isSuggestable? (To disambiguate between fixes and suggestions, it's probably better to keep the naming in line.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ugh... I always mix them up...
fixed (pun intended 😄)

@nzakas nzakas added accepted There is consensus among the team that this change meets the criteria for inclusion contributor pool labels Feb 9, 2024
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

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

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

@nzakas nzakas merged commit 74124c2 into eslint:main Feb 14, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion contributor pool feature This change adds a new feature to ESLint rule Relates to ESLint's core rules
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

Rule Change: Add suggestions to use-isnan
4 participants