Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.8k
fix(eslint-plugin): [no-useless-template-literals] report Infinity & NaN#8295
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
fix(eslint-plugin): [no-useless-template-literals] report Infinity & NaN#8295
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Thanks for the PR,@StyleShit! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently onhttps://opencollective.com/typescript-eslint. |
netlifybot commentedJan 23, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
✅ Deploy Preview fortypescript-eslint ready!
To edit notification comments on pull requests, go to yourNetlify site configuration. |
caf2dac to3158dadCompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM, thanks for the quick + straightforward fix! 🧹 ✨
A couple of small thoughts, nothing blocking. Adding1 approval and setting a reminder to get to this before our next release.
| expression.type === AST_NODE_TYPES.Identifier && | ||
| expression.name === 'NaN' | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[Style] Nit: both of these functions checkexpression.type === AST_NODE_TYPES.Identifier. I think it'd be just a bit cleaner code to avoid the duplicate check and instead have a single function for both. WDYT?
This is a super nitpicky nit and definitely not a blocker 😄.
A good potential followup might be to look into writing a general catch-all function for this. Searchingexpression.type === AST_NODE_TYPES.Identifier shows a few dozen results. Maybe half or more of them or so are similar calls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah, so I basically took inspiration from this:
| functionisIdentifier( | |
| init:TSESTree.Expression, | |
| ...names:string[] | |
| ):boolean{ | |
| return( | |
| init.type===AST_NODE_TYPES.Identifier&&names.includes(init.name) | |
| ); | |
| } |
And thought about extracting it to a util, WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Probably useful to have that as a util yeah. I'm not super picky here to be honest.
packages/eslint-plugin/tests/rules/no-useless-template-literals.test.ts OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
…NaN (typescript-eslint#8295)* fix(eslint-plugin): [no-useless-template-literals] report Infinity & NaNClosestypescript-eslint#8294* remove unnecessary tests
Closes#8294