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

Rule proposal: Use named capture groups for regex #11381

Closed
Assignees
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

Comments

@sindresorhus
Copy link
Contributor

sindresorhus commented Feb 12, 2019

Please describe what the rule should do:

Enforce using named capture groups for regexes instead numbered capture groups. Named capture groups make regexes more readable and they can also prevent possible off-by-one errors.

What category of rule is this? (place an "X" next to just one item)

[ ] Warns about a potential error (problem)
[x] Suggests an alternate way of doing something (suggestion)
[ ] Enforces code style (layout)
[ ] Other (please specify:)

Provide 2-3 code examples that this rule will warn about:

/([0-9]{4})-([0-9]{2})-([0-9]{2})/;
/Chapter (\d+)\.\d*/;

For reference, the first example would look like this with named capture groups:

/(?<year>[0-9]{4})-(?<month>[0-9]{2})-(?<day>[0-9]{2})/;

Why should this rule be included in ESLint (instead of a plugin)?

It's a native JS feature and ESLint already has some other regex-related rules.

Are you willing to submit a pull request to implement this rule?

No

@sindresorhus sindresorhus added feature This change adds a new feature to ESLint rule Relates to ESLint's core rules triage An ESLint team member will look at this issue soon labels Feb 12, 2019
@g-plane g-plane added evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Feb 12, 2019
@platinumazure
Copy link
Member

I'm in favor of this in general, although I think I would characterize this as a suggestion rather than a problem (i.e., the rule doesn't report a known/very likely actual bug, but rather can help avoid a type of bug that might or might not occur in a project).

@sindresorhus
Copy link
Contributor Author

@platinumazure Yeah, I was not sure about that. It could potentially catch a problem, but you're right, the main intention is readability. Changed.

@ilyavolodin
Copy link
Member

Anyone wants to champion this rule? It has enough up-votes to be accepted, just need a champion.

@mysticatea
Copy link
Member

@mysticatea mysticatea self-assigned this Feb 13, 2019
@mysticatea mysticatea added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Feb 13, 2019
@not-an-aardvark
Copy link
Member

Sometimes, people use parentheses to indicate grouping in a regex without using it as a capture group, e.g.

const regex = /(ab|cd|ef)+/;

Would this report an error? It could be replaced with /(?:ab|cd|ef)+/, although maybe some users would think using a non-capturing group is unnecessary. (Numbered capture groups are generally only a readability issue if the groups are actually accessed on a match.)

(This comment is cross-posted from #11392 (comment) for more visibility.)

@sindresorhus
Copy link
Contributor Author

Would this report an error?

IMHO, yes, but if you think it's a big problem, there could be an option to opt-out of that.

platinumazure pushed a commit that referenced this issue Mar 2, 2019
* New: add rule "require-named-capture-group" (fixes #11381)

* Chore: fix linting errors

* Update: change rule name

* Update: use `ReferenceTracker`

* Update: ignore regexp syntax errors

* Update: detect "unicode" flag

* Update: report group instead of AST node

* Docs: improve words

* Chore: enable early return

* Update: improve the message

* Chore: simplify message

Co-Authored-By: g-plane <g-plane@hotmail.com>
@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.