-
Notifications
You must be signed in to change notification settings - Fork 11
Add regexp/grapheme-string-literal
rule
#646
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
🦋 Changeset detectedLatest commit: 331159a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Same problem as #642. |
code: String.raw`/[\q{abc}]/v`, | ||
errors: [ | ||
{ | ||
message: "Use single grapheme in string literal.", |
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 error message needs to be improved. If I didn't read the docs for the rule, I would have no idea what the error message wants me to do.
Maybe we could say this: "Only single characters and graphemes are allowed inside character classes. Use regular alternatives (e.g. (?:foo|[...])
) for strings instead."
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.
Thank you! I will use that message.
lib/rules/grapheme-string-literal.ts
Outdated
meta: { | ||
docs: { | ||
description: "enforce single grapheme in string literal", | ||
category: "Best Practices", |
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 not sure whether this should be a best practice. The practice this rule enforces is good, because it brings regexes into a canonical form (e.g. don't use [\q{foo|bar}]
, use (?:foo|bar)
), but that's more of a stylistic thing IMO.
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 agree with you 👍 I will categorize it under "Stylistic Issues".
close #644