-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
New: require-unicode-regexp rule (fixes #9961) #10698
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.
Left a few minor comments, but this is looking great!
lib/rules/require-unicode-regexp.js
Outdated
url: "https://eslint.org/docs/rules/require-unicode-regexp" | ||
}, | ||
messages: { | ||
requireUFlag: "Require 'u' flag." |
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 think this could be phrased better:
"'u' flag is required."
Use the 'u' flag."
errors: [{ messageId: "requireUFlag" }] | ||
}, | ||
{ | ||
code: "const flags = 'gimu'; new RegExp('foo', flags.slice(0, -1))", |
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'm confused. The documentation says this test should be valid (i.e., rule ignores these cases where flags is non-constant variable), but this test is in the invalid section. Seems either this or the documentation needs to change.
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.
My word choice seemed bad. That ignores a RegExp
call if it couldn't evaluate the 2nd argument to a static value in the file. In the test case, flags.slice(0, -1)
is evaluated to 'gim'
under the assumption that the standard APIs are valid.
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.
Initially I assumed this wouldn't be determined statically, but I spent a few minutes looking at your eslint-utils
module, specifically the getStaticValue
implementation, and I am thoroughly impressed with how powerful it is 👏
I updated the error message and the documentation. |
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.
LGTM, thanks!
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.
LGTM, thanks!
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.
Two nits, but the implementation and tests look great! Neither nit is worthy of preventing merging if you don't get to them. I'm impressed with how thorough the static analysis is in the constructor/call case!
docs/rules/require-unicode-regexp.md
Outdated
@@ -0,0 +1,55 @@ | |||
# enforce the use of `u` flag on RegExp (require-unicode-regexp) |
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.
Tiny nit: can you capitalize the first letter of "Enforce"?
lib/rules/require-unicode-regexp.js
Outdated
@@ -0,0 +1,65 @@ | |||
/** | |||
* @fileoverview Rule to |
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.
The @fileoverview
description is incomplete
const c = new RegExp("ccc", "u") | ||
const d = new RegExp("ddd", "giu") | ||
|
||
// This rule ignores RegExp calls if the flags could not be evaluated to a static value. |
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.
Good idea to clarify this 👏
errors: [{ messageId: "requireUFlag" }] | ||
}, | ||
{ | ||
code: "const flags = 'gimu'; new RegExp('foo', flags.slice(0, -1))", |
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.
Initially I assumed this wouldn't be determined statically, but I spent a few minutes looking at your eslint-utils
module, specifically the getStaticValue
implementation, and I am thoroughly impressed with how powerful it is 👏
What is the purpose of this pull request? (put an "X" next to item)
[X] New rule: fixes #9961.
What changes did you make? (Give an overview)
This PR adds
require-unicode-regex
rule.Is there anything you'd like reviewers to focus on?
Nothing in particular.