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: rule no-unsafe-values #30

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

bmeck
Copy link

@bmeck bmeck commented Oct 3, 2024

Prerequisites checklist

What is the purpose of this pull request?

Introduce a new rule for checking common interchange problems

What changes did you make? (Give an overview)

Added a rule for:

  • non-finite (Infinity, null, NaN) values.
  • lone surrogates due to UTF16

Related Issues

fixes #29

Is there anything you'd like reviewers to focus on?

tests are somewhat limited by using already parsed values so I avoided trying to reparse for getting exact locations within strings and/or couldn't check array/string bounds since things error before getting to the rules processing phase.

Copy link

linux-foundation-easycla bot commented Oct 3, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

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.

This looks like a good start. Some high-level notes:

  1. Please add this rule to the rules table on the README
  2. Please add this rule to the recommended configuration

Also, it looks like your last invalid test is actually producing two errors instead of one. Can you double-check that?

if (match[0].length < 2) {
context.report({
loc: node.loc,
messageId: "loneSurrogate",
Copy link
Member

Choose a reason for hiding this comment

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

Because we're not showing the actual location of the lone surrogate, can we show the surrogate itself? That would give folks a bit more information about where the problem is.

Copy link
Author

Choose a reason for hiding this comment

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

sure

src/rules/no-unsafe-values.js Outdated Show resolved Hide resolved
bmeck and others added 2 commits October 7, 2024 17:42
Comment on lines +67 to +73
{
messageId: "loneSurrogate",
line: 1,
column: 1,
endLine: 1,
endColumn: 4,
},
Copy link
Member

Choose a reason for hiding this comment

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

Can you add data to all test errors with messageId: "loneSurrogate"?

@nzakas
Copy link
Member

nzakas commented Oct 18, 2024

@bmeck just one outstanding comment to address here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New Rule: unsafe values
3 participants