-
-
Notifications
You must be signed in to change notification settings - Fork 3
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
Conversation
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.
This looks like a good start. Some high-level notes:
- Please add this rule to the rules table on the README
- 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", |
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.
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.
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.
sure
Co-authored-by: Nicholas C. Zakas <[email protected]>
{ | ||
messageId: "loneSurrogate", | ||
line: 1, | ||
column: 1, | ||
endLine: 1, | ||
endColumn: 4, | ||
}, |
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.
Can you add data
to all test errors with messageId: "loneSurrogate"
?
@bmeck just one outstanding comment to address here. |
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.
Because we're just waiting for a change to the tests, I'll merge this and then update the tests myself.
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:
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.