feat: `require-unicode-regexp` add suggestions #17007

JoshuaKGoldberg · 2023-03-21T21:15:05Z

Prerequisites checklist

I have read the contributing guidelines.

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)
[x] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

Fixes #15089.

What changes did you make? (Give an overview)

Adds a suggest array to the fixes for the require-unicode-regexp rule.

Is there anything you'd like reviewers to focus on?

🤷

netlify · 2023-03-21T21:16:45Z

✅ Deploy Preview for docs-eslint ready!

Name	Link
🔨 Latest commit	`339056a`
🔍 Latest deploy log	https://app.netlify.com/sites/docs-eslint/deploys/642248070e9536000832d9f7
😎 Deploy Preview	https://deploy-preview-17007--docs-eslint.netlify.app/rules/semi
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

mdjermanovic

Thanks for working on this!

lib/rules/require-unicode-regexp.js

lib/rules/utils/unicode/regular-expressions.js

fasttime · 2023-03-22T17:54:31Z

Thanks @JoshuaKGoldberg! For new RegExp('foo', false) I am getting an inconsistent suggestion. The suggested change is new RegExp('foo', falsue). Can we adjust this somehow, or remove the suggestion?

lib/rules/require-unicode-regexp.js

fasttime · 2023-03-23T07:08:29Z

For RegExp('', a = 'g'), the suggested change is RegExp('', a = 'g' + "u"), which makes sense, but maybe RegExp('', (a = 'g') + "u") would be a better choice because of reduced side effects.

Anyway, I don't see any unit tests where the second argument of RegExp is an assignment. Shall we add some?

lib/rules/utils/unicode/regular-expressions.js

mdjermanovic · 2023-03-23T09:27:40Z

For RegExp('', a = 'g'), the suggested change is RegExp('', a = 'g' + "u"), which makes sense, but maybe RegExp('', (a = 'g') + "u") would be a better choice because of reduced side effects.

I think it might be best to just not provide suggestions that add + "u". For example, if the code is const flags = "g"; const regex = new RegExp("foo", flags);, user would probably want const flags = "gu"; rather than new RegExp("foo", flags + "u");.

lib/rules/require-unicode-regexp.js

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

lib/rules/utils/regular-expressions.js

JoshuaKGoldberg · 2023-03-23T17:40:43Z

Test failure is unrelated:

  1) CLIEngine
       "before all" hook in "CLIEngine":
     Error: Timeout of 60000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (D:\a\eslint\eslint\tests\lib\cli-engine\cli-engine.js)
      at process.processImmediate (node:internal/timers:476:21)

fasttime

Noticed that we have an existing false positive with RegExp(...patternAndFlags). If we don't fix this here, we should do it in a new PR.

lib/rules/utils/regular-expressions.js

fasttime

LGTM!

nzakas

LGTM. Would like @mdjermanovic to verify his concerns were addressed.

mdjermanovic

LGTM, thanks!

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [eslint](https://eslint.org) ([source](https://github.com/eslint/eslint)) | devDependencies | minor | [`8.36.0` -> `8.37.0`](https://renovatebot.com/diffs/npm/eslint/8.36.0/8.37.0) | --- ### Release Notes <details> <summary>eslint/eslint</summary> ### [`v8.37.0`](https://github.com/eslint/eslint/releases/tag/v8.37.0) [Compare Source](eslint/eslint@v8.36.0...v8.37.0) #### Features - [`b6ab8b2`](eslint/eslint@b6ab8b2) feat: `require-unicode-regexp` add suggestions ([#17007](eslint/eslint#17007)) (Josh Goldberg) - [`10022b1`](eslint/eslint@10022b1) feat: Copy getScope() to SourceCode ([#17004](eslint/eslint#17004)) (Nicholas C. Zakas) - [`1665c02`](eslint/eslint@1665c02) feat: Use plugin metadata for flat config serialization ([#16992](eslint/eslint#16992)) (Nicholas C. Zakas) - [`b3634f6`](eslint/eslint@b3634f6) feat: docs license ([#17010](eslint/eslint#17010)) (Samuel Roldan) - [`892e6e5`](eslint/eslint@892e6e5) feat: languageOptions.parser must be an object. ([#16985](eslint/eslint#16985)) (Nicholas C. Zakas) #### Bug Fixes - [`619f3fd`](eslint/eslint@619f3fd) fix: correctly handle `null` default config in `RuleTester` ([#17023](eslint/eslint#17023)) (Brad Zacher) - [`1fbf118`](eslint/eslint@1fbf118) fix: `getFirstToken`/`getLastToken` on comment-only node ([#16889](eslint/eslint#16889)) (Francesco Trotta) - [`129e252`](eslint/eslint@129e252) fix: Fix typo in `logical-assignment-operators` rule description ([#17000](eslint/eslint#17000)) (Francesco Trotta) #### Documentation - [`75339df`](eslint/eslint@75339df) docs: fix typos and missing info in id-match docs ([#17029](eslint/eslint#17029)) (Ed Lucas) - [`ec2d830`](eslint/eslint@ec2d830) docs: Fix typos in the `semi` rule docs ([#17012](eslint/eslint#17012)) (Andrii Lundiak) - [`e39f28d`](eslint/eslint@e39f28d) docs: add back to top button ([#16979](eslint/eslint#16979)) (Tanuj Kanti) - [`721c717`](eslint/eslint@721c717) docs: Custom Processors cleanup and expansion ([#16838](eslint/eslint#16838)) (Ben Perlmutter) - [`d049f97`](eslint/eslint@d049f97) docs: 'How ESLint is Maintained' page ([#16961](eslint/eslint#16961)) (Ben Perlmutter) - [`5251a92`](eslint/eslint@5251a92) docs: Describe guard options for guard-for-in ([#16986](eslint/eslint#16986)) (alope107) - [`6157d81`](eslint/eslint@6157d81) docs: Add example to guard-for-in docs. ([#16983](eslint/eslint#16983)) (alope107) - [`fd47998`](eslint/eslint@fd47998) docs: update `Array.prototype.toSorted` specification link ([#16982](eslint/eslint#16982)) (Milos Djermanovic) - [`3e1cf6b`](eslint/eslint@3e1cf6b) docs: Copy edits on Maintain ESLint docs ([#16939](eslint/eslint#16939)) (Ben Perlmutter) #### Chores - [`c67f299`](eslint/eslint@c67f299) chore: upgrade [@eslint/js](https://github.com/eslint/js)[@8](https://github.com/8).37.0 ([#17033](eslint/eslint#17033)) (Milos Djermanovic) - [`ee9ddbd`](eslint/eslint@ee9ddbd) chore: package.json update for [@eslint/js](https://github.com/eslint/js) release (ESLint Jenkins) - [`dddb475`](eslint/eslint@dddb475) chore: upgrade [@eslint/eslintrc](https://github.com/eslint/eslintrc)[@2](https://github.com/2).0.2 ([#17032](eslint/eslint#17032)) (Milos Djermanovic) - [`522431e`](eslint/eslint@522431e) chore: upgrade espree@9.5.1 ([#17031](eslint/eslint#17031)) (Milos Djermanovic) - [`f5f9a88`](eslint/eslint@f5f9a88) chore: upgrade eslint-visitor-keys@3.4.0 ([#17030](eslint/eslint#17030)) (Milos Djermanovic) - [`4dd8d52`](eslint/eslint@4dd8d52) ci: bump actions/stale from 7 to 8 ([#17026](eslint/eslint#17026)) (dependabot\[bot]) - [`ad9dd6a`](eslint/eslint@ad9dd6a) chore: remove duplicate scss, ([#17005](eslint/eslint#17005)) (Strek) - [`ada6a3e`](eslint/eslint@ada6a3e) ci: unpin Node 19 ([#16993](eslint/eslint#16993)) (Milos Djermanovic) - [`c3da975`](eslint/eslint@c3da975) chore: Remove triage label from template ([#16990](eslint/eslint#16990)) (Nicholas C. Zakas) - [`69bc0e2`](eslint/eslint@69bc0e2) ci: pin Node 19 to 19.7.0 ([#16987](eslint/eslint#16987)) (Milos Djermanovic) </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. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1834 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>

feat: require-unicode-regexp add suggestions

af13e12

JoshuaKGoldberg requested a review from a team as a code owner March 21, 2023 21:15

eslint-github-bot bot added triage An ESLint team member will look at this issue soon feature This change adds a new feature to ESLint labels Mar 21, 2023

mdjermanovic added rule Relates to ESLint's core rules accepted There is consensus among the team that this change meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Mar 22, 2023

mdjermanovic requested changes Mar 22, 2023

View reviewed changes

lib/rules/require-unicode-regexp.js Outdated Show resolved Hide resolved

lib/rules/require-unicode-regexp.js Outdated Show resolved Hide resolved

Review fixups: invalid patterns; sequence expressions

238ea0d

JoshuaKGoldberg commented Mar 22, 2023

View reviewed changes

lib/rules/utils/unicode/regular-expressions.js Outdated Show resolved Hide resolved

JoshuaKGoldberg marked this pull request as draft March 22, 2023 17:41

I promise I know how CJS modules work

114c58a

JoshuaKGoldberg marked this pull request as ready for review March 22, 2023 17:49

JoshuaKGoldberg requested a review from mdjermanovic March 22, 2023 17:49

JoshuaKGoldberg commented Mar 22, 2023

View reviewed changes

lib/rules/require-unicode-regexp.js Show resolved Hide resolved

Handled non-string literals

bfc0bae

mdjermanovic reviewed Mar 23, 2023

View reviewed changes

lib/rules/utils/unicode/regular-expressions.js Outdated Show resolved Hide resolved

mdjermanovic reviewed Mar 23, 2023

View reviewed changes

lib/rules/require-unicode-regexp.js Outdated Show resolved Hide resolved

JoshuaKGoldberg and others added 3 commits March 23, 2023 08:25

Update lib/rules/require-unicode-regexp.js

99876d7

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

Don't concatenate + 'u'

adb304e

Test the new cases

20e4feb

mdjermanovic reviewed Mar 23, 2023

View reviewed changes

lib/rules/utils/regular-expressions.js Outdated Show resolved Hide resolved

Touch up regular-expressions.js templating

c1243c9

fasttime reviewed Mar 24, 2023

View reviewed changes

lib/rules/utils/regular-expressions.js Show resolved Hide resolved

Add myself as author to regular-expressions.js

36c3484

Merge branch 'main'

339056a

fasttime approved these changes Mar 28, 2023

View reviewed changes

nzakas approved these changes Mar 28, 2023

View reviewed changes

mdjermanovic added the contributor pool label Mar 28, 2023

mdjermanovic approved these changes Mar 28, 2023

View reviewed changes

mdjermanovic merged commit b6ab8b2 into eslint:main Mar 28, 2023
21 checks passed

JoshuaKGoldberg deleted the require-unicode-regexp-suggestions branch March 28, 2023 19:23

fasttime mentioned this pull request Mar 29, 2023

fix: do not report on RegExp(...args) in require-unicode-regexp #17037

Merged

1 task

github-actions bot mentioned this pull request May 2, 2023

Upgraded some dependencies - Attempt 2 AurorNZ/report-updated-dependencies#638

Merged

eslint-github-bot bot locked and limited conversation to collaborators Sep 25, 2023

eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Sep 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: `require-unicode-regexp` add suggestions #17007

feat: `require-unicode-regexp` add suggestions #17007

JoshuaKGoldberg commented Mar 21, 2023 •

edited

netlify bot commented Mar 21, 2023 •

edited

mdjermanovic left a comment

fasttime commented Mar 22, 2023

fasttime commented Mar 23, 2023

mdjermanovic commented Mar 23, 2023

JoshuaKGoldberg commented Mar 23, 2023

fasttime left a comment

fasttime left a comment

nzakas left a comment

mdjermanovic left a comment

feat: require-unicode-regexp add suggestions #17007

feat: require-unicode-regexp add suggestions #17007

Conversation

JoshuaKGoldberg commented Mar 21, 2023 • edited

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

What changes did you make? (Give an overview)

Is there anything you'd like reviewers to focus on?

netlify bot commented Mar 21, 2023 • edited

✅ Deploy Preview for docs-eslint ready!

mdjermanovic left a comment

Choose a reason for hiding this comment

fasttime commented Mar 22, 2023

fasttime commented Mar 23, 2023

mdjermanovic commented Mar 23, 2023

JoshuaKGoldberg commented Mar 23, 2023

fasttime left a comment

Choose a reason for hiding this comment

fasttime left a comment

Choose a reason for hiding this comment

nzakas left a comment

Choose a reason for hiding this comment

mdjermanovic left a comment

Choose a reason for hiding this comment

feat: `require-unicode-regexp` add suggestions #17007

feat: `require-unicode-regexp` add suggestions #17007

JoshuaKGoldberg commented Mar 21, 2023 •

edited

netlify bot commented Mar 21, 2023 •

edited