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

fix(eslint-plugin): [no-useless-template-literals] report Infinity & NaN #8295

Conversation

StyleShit
Copy link
Contributor

Closes #8294

@typescript-eslint
Copy link
Contributor

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 on https://opencollective.com/typescript-eslint.

Copy link

netlify bot commented Jan 23, 2024

Deploy Preview for typescript-eslint ready!

Name Link
🔨 Latest commit b0bc73d
🔍 Latest deploy log https://app.netlify.com/sites/typescript-eslint/deploys/65b1f444e52f120008fed03c
😎 Deploy Preview https://deploy-preview-8295--typescript-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 96 (🟢 up 6 from production)
Accessibility: 100 (no change from production)
Best Practices: 92 (no change from production)
SEO: 98 (no change from production)
PWA: 80 (no change from production)
View the detailed breakdown and full score reports

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

@StyleShit StyleShit force-pushed the fix/no-useless-template-literals-infinity-nan branch from caf2dac to 3158dad Compare January 23, 2024 18:54
@StyleShit StyleShit changed the title fix(eslint-plugin): [no-useless-template-literals] Report Infinity & NaN fix(eslint-plugin): [no-useless-template-literals] report Infinity & NaN Jan 23, 2024
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg 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 for the quick + straightforward fix! 🧹 ✨

A couple of small thoughts, nothing blocking. Adding 1 approval and setting a reminder to get to this before our next release.

expression.type === AST_NODE_TYPES.Identifier &&
expression.name === 'NaN'
);
}
Copy link
Member

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 check expression.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. Searching expression.type === AST_NODE_TYPES.Identifier shows a few dozen results. Maybe half or more of them or so are similar calls.

Copy link
Contributor Author

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:

function isIdentifier(
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?

Copy link
Member

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.

@JoshuaKGoldberg JoshuaKGoldberg added the 1 approval PR that a maintainer has LGTM'd - any maintainer can merge this when ready label Jan 24, 2024
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

:shipit:

@JoshuaKGoldberg JoshuaKGoldberg merged commit d02d086 into typescript-eslint:main Jan 29, 2024
56 of 58 checks passed
danvk pushed a commit to danvk/typescript-eslint that referenced this pull request Feb 4, 2024
…NaN (typescript-eslint#8295)

* fix(eslint-plugin): [no-useless-template-literals] report Infinity & NaN

Closes typescript-eslint#8294

* remove unnecessary tests
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1 approval PR that a maintainer has LGTM'd - any maintainer can merge this when ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: [no-useless-template-literals] Missing report for Infinity and NaN
2 participants