Skip to content

feat: require-unicode-regexp support v flag#17402

Merged
nzakas merged 13 commits into
eslint:mainfrom
sosukesuzuki:17223
Jul 28, 2023
Merged

feat: require-unicode-regexp support v flag#17402
nzakas merged 13 commits into
eslint:mainfrom
sosukesuzuki:17223

Conversation

@sosukesuzuki

@sosukesuzuki sosukesuzuki commented Jul 22, 2023 •
edited by mdjermanovic
Loading

Copy link
Copy Markdown
Contributor

Prerequisites checklist

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

[x] Documentation update
[x] Changes an existing rule (template)
[x] Add autofix to a rule

What changes did you make? (Give an overview)

Refs #17223

  • The current require-unicode-regexp warns if there's no u flag, but we have updated it to also allow the v flag.
  • Add a new option requiredUnicodeFlag: "u" | "v". When this option is specified, a warning is issued if the designated flag is not included.

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

Option interface.

@eslint-github-bot

eslint-github-bot Bot commented Jul 22, 2023

Copy link
Copy Markdown

Hi @sosukesuzuki!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

  • The commit message tag wasn't recognized. Did you mean "docs", "fix", or "feat"?
  • There should be a space following the initial tag and colon, for example 'feat: Message'.
  • The first letter of the tag should be in lowercase
  • The length of the commit message must be less than or equal to 72

To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page.

Read more about contributing to ESLint here

@netlify

netlify Bot commented Jul 22, 2023 •
edited
Loading

Copy link
Copy Markdown

✅ Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit 93b828e
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/64c3e06f642b320008727847

@sosukesuzuki sosukesuzuki changed the title Add requiredUnicodeFlag option to require-unicode-regexp and support v flag feat: add requiredUnicodeFlag option to require-unicode-regexp and support v flag Jul 22, 2023
@eslint-github-bot

eslint-github-bot Bot commented Jul 22, 2023

Copy link
Copy Markdown

Hi @sosukesuzuki!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

  • The length of the commit message must be less than or equal to 72

To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page.

Read more about contributing to ESLint here

@sosukesuzuki sosukesuzuki changed the title feat: add requiredUnicodeFlag option to require-unicode-regexp and support v flag feat: require-unicode-regexp support v flag Jul 22, 2023
@eslint-github-bot eslint-github-bot Bot added the feature This change adds a new feature to ESLint label Jul 22, 2023
@sosukesuzuki sosukesuzuki marked this pull request as ready for review July 22, 2023 09:14
@sosukesuzuki sosukesuzuki requested a review from a team as a code owner July 22, 2023 09:14
mdjermanovic
mdjermanovic reviewed Jul 22, 2023
View reviewed changes
Comment thread lib/rules/require-unicode-regexp.js Outdated
@mdjermanovic mdjermanovic added rule Relates to ESLint's core rules accepted There is consensus among the team that this change meets the criteria for inclusion new syntax This issue is related to new syntax that has reached stage 4 labels Jul 22, 2023
@mdjermanovic mdjermanovic mentioned this pull request Jul 22, 2023
Closed
19 tasks
@mdjermanovic

mdjermanovic commented Jul 22, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

I've edited the original post to change Fixes #17223 to Refs #17223 so that #17223 doesn't get automatically closed when we merge this, as it's unlikely that this is the only rule that needs to be updated.

mdjermanovic
mdjermanovic reviewed Jul 24, 2023
View reviewed changes

@mdjermanovic mdjermanovic left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These lines in the docs should also be updated with the v flag:

eslint/docs/src/rules/require-unicode-regexp.md

Line 28 in da73e58

This rule aims to enforce the use of `u` flag on regular expressions.

eslint/docs/src/rules/require-unicode-regexp.md

Line 67 in da73e58

If you don't want to notify regular expressions with no `u` flag, then it's safe to disable this rule.

I also left a few other small suggestions.

Comment thread docs/src/rules/require-unicode-regexp.md Outdated
Comment thread lib/rules/require-unicode-regexp.js Outdated
Comment thread tests/lib/rules/require-unicode-regexp.js Outdated
Comment thread tests/lib/rules/require-unicode-regexp.js Outdated
nzakas
nzakas reviewed Jul 24, 2023
View reviewed changes
Comment thread docs/src/rules/require-unicode-regexp.md
re.test('\u2028'); // → false
```

Please see <https://github.com/tc39/proposal-regexp-v-flag> and <https://v8.dev/features/regexp-v-flag> for more details.

@nzakas nzakas Jul 24, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These links should go in a "Further Reading" section at the bottom of the page instead of here. See the semi rule as an example: https://eslint.org/docs/latest/rules/semi#further-reading

@mdjermanovic

mdjermanovic commented Jul 28, 2023

Copy link
Copy Markdown
Member

Changes in the code LGTM. If review comments aren't addressed in time for today's release, I'm fine with merging this as-is to complete #17223, and addressing reviews later in a follow-up.

sosukesuzuki and others added 3 commits July 29, 2023 00:23
@sosukesuzuki @mdjermanovic
Update lib/rules/require-unicode-regexp.js
144f73d 
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
@sosukesuzuki @mdjermanovic
Update docs/src/rules/require-unicode-regexp.md
713c0c7 
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
@sosukesuzuki @mdjermanovic
Update tests/lib/rules/require-unicode-regexp.js
082a792 
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
@sosukesuzuki @mdjermanovic
Update tests/lib/rules/require-unicode-regexp.js
93b828e 
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
nzakas
nzakas approved these changes Jul 28, 2023
View reviewed changes

@nzakas nzakas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approving for this release even though we need to clean up the documentation a bit later.

@nzakas nzakas merged commit 8a93438 into eslint:main Jul 28, 2023
@sosukesuzuki sosukesuzuki deleted the 17223 branch August 1, 2023 16:39
@sosukesuzuki sosukesuzuki mentioned this pull request Aug 1, 2023
Merged
1 task
nzakas added a commit that referenced this pull request Aug 2, 2023
@nzakas
docs: update require-unicode-regexp.md as following up #17402 (#17441)
47a0859 
* docs: add `further_reading` links

* docs: mention about `v`

* Update docs/src/rules/require-unicode-regexp.md

---------

Co-authored-by: Nicholas C. Zakas <nicholas@humanwhocodes.com>
@eslint-github-bot eslint-github-bot Bot locked and limited conversation to collaborators Jan 25, 2024
@eslint-github-bot 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 Jan 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@mdjermanovic mdjermanovic mdjermanovic left review comments
@nzakas nzakas nzakas approved these changes

Assignees

No one assigned

Labels

accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion contributor pool feature This change adds a new feature to ESLint new syntax This issue is related to new syntax that has reached stage 4 rule Relates to ESLint's core rules

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sosukesuzuki @mdjermanovic @nzakas