-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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
tools: add ASCII only lint rule in lib/ #11371
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,3 +3,4 @@ rules: | |
require-buffer: 2 | ||
buffer-constructor: 2 | ||
no-let-in-for-declaration: 2 | ||
only-ascii-characters: 2 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,65 @@ | ||
/** | ||
* @fileoverview Prohibit the use of non-ascii characters | ||
* @author Kalon Hinds | ||
*/ | ||
|
||
/* eslint no-control-regex:0 */ | ||
|
||
'use strict'; | ||
|
||
//------------------------------------------------------------------------------ | ||
// Rule Definition | ||
//------------------------------------------------------------------------------ | ||
|
||
const nonAsciiPattern = new RegExp('([^\x00-\x7F])', 'g'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be clearer to use a regex literal here instead of the |
||
const suggestions = { | ||
'’': '\'', | ||
'—': '-' | ||
}; | ||
const reportError = ({line, column, character}, node, context) => { | ||
const suggestion = suggestions[character]; | ||
|
||
let message = `Non-ASCII character ${character} detected.`; | ||
|
||
message = suggestion ? | ||
`${message} Consider replacing with: ${suggestion}` : message; | ||
|
||
context.report({ | ||
node, | ||
message, | ||
loc: { | ||
line, | ||
column | ||
} | ||
}); | ||
}; | ||
|
||
module.exports = { | ||
create: (context) => { | ||
return { | ||
Program: (node) => { | ||
const source = context.getSourceCode(); | ||
const sourceTokens = source.getTokens(node); | ||
const commentTokens = source.getAllComments(); | ||
const tokens = sourceTokens.concat(commentTokens); | ||
|
||
tokens.forEach((token) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will fail to match non-ascii whitespace that could appear between tokens, so it's not quite disallowing all non-ascii characters in files. This could be fixed by matching the regex against That said, I think the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the latter is true, it would be good to have a comment explaining that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment alone would not be sufficient, it is important that the linter ensures there are no irregular Unicode whitespace characters since they cannot be seen during code review. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @aqrln by:
I mean that if:
@not-an-aardvark's theory is correct, and the non-ASCII whitespace characters are already covered in a separate rule, then we could just use that for whitespace, and add a comment in here to explain that we don't need to worry about whitespace as it's covered in another rule. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @gibfahn ah, I see, sorry. I didn't pay enough attention to that "if the latter" part so I didn't understand you right. |
||
const { value } = token; | ||
const matches = value.match(nonAsciiPattern); | ||
|
||
if (!matches) return; | ||
|
||
const { loc } = token; | ||
const character = matches[0]; | ||
const column = loc.start.column + value.indexOf(character); | ||
|
||
reportError({ | ||
line: loc.start.line, | ||
column, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This could result in an invalid report location if the offending character is in a block comment. For example: /* foo
■ */ The rule reports an error for this comment at line 1, column 7, but that location doesn't actually exist. |
||
character | ||
}, node, context); | ||
}); | ||
} | ||
}; | ||
} | ||
}; |
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.
I'd prefer see
eslint-disable-line
oreslint-disable-next-line
to target the places where the control characters are needed.