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

New: add rule "prefer-named-capture-group" (fixes #11381) #11392

Merged
merged 11 commits into from Mar 2, 2019

Conversation

g-plane
Copy link
Member

@g-plane g-plane commented Feb 14, 2019

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

[x] New rule

What changes did you make? (Give an overview)
Added a new rule called "require-named-capture-group".
Fixes #11381

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

  1. This is my first time to create a new core rule to ESLint, so I'm welcome any changes requesting.
  2. I'm not sure the rule name is good enough, and I'm opening for discussing about improving the rule name.

@g-plane g-plane added rule Relates to ESLint's core rules accepted There is consensus among the team that this change meets the criteria for inclusion feature This change adds a new feature to ESLint labels Feb 14, 2019
Copy link
Member

@mysticatea mysticatea left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

The direction looks good to me.
But I have some suggestions, would you address those?

docs/rules/require-named-capture-group.md Outdated Show resolved Hide resolved
lib/rules/require-named-capture-group.js Outdated Show resolved Hide resolved
lib/rules/require-named-capture-group.js Outdated Show resolved Hide resolved
lib/rules/require-named-capture-group.js Outdated Show resolved Hide resolved
Copy link
Member

@mysticatea mysticatea left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@mysticatea mysticatea changed the title New: add rule "require-named-capture-group" (fixes #11381) New: add rule "prefer-named-capture-group" (fixes #11381) Feb 19, 2019
Copy link
Member

@not-an-aardvark not-an-aardvark left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! I have a few suggestions/questions.

docs/rules/prefer-named-capture-group.md Outdated Show resolved Hide resolved
docs/rules/prefer-named-capture-group.md Outdated Show resolved Hide resolved
lib/rules/prefer-named-capture-group.js Show resolved Hide resolved
tests/lib/rules/prefer-named-capture-group.js Show resolved Hide resolved
Copy link
Member

@not-an-aardvark not-an-aardvark left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

LGTM outside of the rule message being a little verbose. I've offered a suggestion but open to tweaks.

lib/rules/prefer-named-capture-group.js Outdated Show resolved Hide resolved
Co-Authored-By: g-plane <g-plane@hotmail.com>
Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

LGTM, thanks so much for making the change!

@platinumazure
Copy link
Member

As a note, the CI is broken due to a bug in the eslint-scope upgrade which we're still investigating. Thanks for your patience!

@platinumazure
Copy link
Member

Closing/reopening to force rerun of CI.

@platinumazure platinumazure reopened this Mar 2, 2019
@platinumazure platinumazure merged commit ec59ec0 into master Mar 2, 2019
@platinumazure platinumazure deleted the issue-11381 branch March 2, 2019 01:51
@platinumazure
Copy link
Member

Merged (at last). Thanks @g-plane for contributing!

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Aug 30, 2019
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Aug 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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 feature This change adds a new feature to ESLint rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rule proposal: Use named capture groups for regex
4 participants