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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 13 commits into from Jul 28, 2023
Merged

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

merged 13 commits into from Jul 28, 2023

Conversation

sosukesuzuki
Copy link
Contributor

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

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
Copy link

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
Copy link

netlify bot commented Jul 22, 2023 •
edited

✅ 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
Copy link

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 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
Copy link
Member

mdjermanovic commented Jul 22, 2023 •
edited

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
Copy link
Member

@mdjermanovic mdjermanovic left a comment •
edited

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.

docs/src/rules/require-unicode-regexp.md Outdated Show resolved Hide resolved
lib/rules/require-unicode-regexp.js Outdated Show resolved Hide resolved
tests/lib/rules/require-unicode-regexp.js Outdated Show resolved Hide resolved
tests/lib/rules/require-unicode-regexp.js Outdated Show resolved Hide resolved
nzakas
nzakas reviewed Jul 24, 2023
View reviewed changes
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.
Copy link
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
Copy link
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
Copy link
Member

@nzakas nzakas left a comment

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
21 checks passed
@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
@sosukesuzuki @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.

None yet
3 participants
@sosukesuzuki @mdjermanovic @nzakas