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: add suggestions for redundant wrapping in prefer-regex-literals #16658
Conversation
|
Name | Link |
---|---|
3043c4c | |
https://app.netlify.com/sites/docs-eslint/deploys/63ac4e11d0e37e0008f3feff |
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.
Functionality looks okay to me but would like @mdjermanovic's opinion.
I left some suggestions for ways to clean up the code a bit.
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 example would have a parsing error after the suggestion is applied:
(a)
new RegExp(/b/);
after applying the suggestion, /
causes continuation so this is parsed as (a) / b /
, and then fails on ;
:
(a)
/b/; // Parsing error: Unexpected token ;
We're preventing this in the branch that already had suggestions, so we could do the same for the new suggestions.
eslint/lib/rules/prefer-regex-literals.js
Lines 335 to 337 in ba74253
const tokenBefore = sourceCode.getTokenBefore(node); | |
if (tokenBefore && !validPrecedingTokens.has(tokenBefore.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.
These are quite edge cases, but we should also use canTokensBeAdjacent
(like we're using it for the existing suggestions in this rule) for correctness:
// 1.
a/RegExp(/foo/);
// 2.
RegExp(/foo/)in a;
After the suggestions are applied, (1) becomes a line comment //foo/
, while in (2) in
becomes the flags.
// 1.
a//foo/;
// 2.
/foo/in a;
@nzakas @mdjermanovic Thanks for the reviews. I fixed! |
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!
Leaving open for @nzakas to check if his reviews have been addressed.
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!
We just have to wait until we get the "all clear" as far as patch releases go, then this PR is ready for merge. |
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [eslint](https://eslint.org) ([source](https://github.com/eslint/eslint)) | devDependencies | minor | [`8.31.0` -> `8.32.0`](https://renovatebot.com/diffs/npm/eslint/8.31.0/8.32.0) | --- ### Release Notes <details> <summary>eslint/eslint</summary> ### [`v8.32.0`](https://github.com/eslint/eslint/releases/tag/v8.32.0) [Compare Source](eslint/eslint@v8.31.0...v8.32.0) #### Features - [`fc20f24`](eslint/eslint@fc20f24) feat: add suggestions for redundant wrapping in prefer-regex-literals ([#​16658](eslint/eslint#16658)) (YeonJuan) #### Bug Fixes - [`b4f8329`](eslint/eslint@b4f8329) fix: ignore directives for no-fallthrough ([#​16757](eslint/eslint#16757)) (gfyoung) #### Documentation - [`17b65ad`](eslint/eslint@17b65ad) docs: IA Update page URL move ([#​16665](eslint/eslint#16665)) (Ben Perlmutter) - [`5981296`](eslint/eslint@5981296) docs: fix theme switcher button ([#​16752](eslint/eslint#16752)) (Sam Chen) - [`6669413`](eslint/eslint@6669413) docs: deploy prerelease docs under the `/docs/next/` path ([#​16541](eslint/eslint#16541)) (Nitin Kumar) - [`78ecfe0`](eslint/eslint@78ecfe0) docs: use inline code for rule options name ([#​16768](eslint/eslint#16768)) (Percy Ma) - [`fc2ea59`](eslint/eslint@fc2ea59) docs: Update README (GitHub Actions Bot) - [`762a872`](eslint/eslint@762a872) docs: Update README (GitHub Actions Bot) #### Chores - [`2952d6e`](eslint/eslint@2952d6e) chore: sync templates/\*.md files with issue templates ([#​16758](eslint/eslint#16758)) (gfyoung) - [`3e34418`](eslint/eslint@3e34418) chore: Add new issues to triage project ([#​16740](eslint/eslint#16740)) (Nicholas C. Zakas) </details> --- ### Configuration📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4xMDIuNCIsInVwZGF0ZWRJblZlciI6IjM0LjEwMi43In0=--> Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1734 Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain: Add suggestions to a rule
What changes did you make? (Give an overview)
fixes #16516
Is there anything you'd like reviewers to focus on?
In this case, this change will make two suggestions. - #16516 (comment)