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

Use eslint-plugin-regexp #144

Merged
merged 8 commits into from
Oct 1, 2024
Merged

Conversation

JLHwung
Copy link
Collaborator

@JLHwung JLHwung commented Sep 27, 2024

In this PR we introduce an ESLint plugin eslint-plugin-regexp and enable most of its recommended rules. Then we fixed all lint errors for each different rules.

This PR is based on #143 so that that PR is also linted against the new ESLint plugin. I will rebase once that PR gets merged.

_ was removed from ClassSetReservedPunctuator

See tc39/proposal-regexp-v-flag#60
Avoid using annexB regexp syntax
\d is shorter than [0-9]
This rule recommends to simplify the following usage

/^[dDsSwW]/ to /^[dsw]/i

This regex is for matching the CharacterClassEscape

// CharacterClassEscape :: one of d D s S w W

If we simplify to the /i flag and suppose CharacterClassEscape now allows a new production `x` but not `X`, it is prone to errors that we accidentally allows `X` because of the `i` flag. For this reason I suggest to disable this rule.
@@ -762,7 +762,7 @@
// PatternCharacter
return createCharacter(res);
}
else if (!isUnicodeMode && (res = matchReg(/^(?:]|})/))) {
else if (!isUnicodeMode && (res = matchReg(/^(?:\]|\})/))) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Aside: Should we introduce an annexB: boolean option? When this option is false, it will reject any annexB productions. We can default it to true to avoid breaking changes, but users can opt-in to the modern stricter regex syntax.

Copy link
Owner

Choose a reason for hiding this comment

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

More than happy to accept a PR implementing this!

@jviereck
Copy link
Owner

Great! Does npm test run the linter automatically? If not, is there a way to trigger it from npm test and make the test fail if there is a lint error?

@JLHwung
Copy link
Collaborator Author

JLHwung commented Sep 27, 2024

Does npm test run the linter automatically?

Yes

"test": "run-p test:* && npm run lint",

@jviereck jviereck merged commit ccc838b into jviereck:gh-pages Oct 1, 2024
@jviereck
Copy link
Owner

jviereck commented Oct 1, 2024

Thanks for this PR @JLHwung ! I will make a release most likely tonight.

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

Successfully merging this pull request may close these issues.

2 participants