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
Fix: prefer-named-capture-group incorrect locations (fixes #12233) #12247
Conversation
Given that we specify the capture group content, I'm wondering if we should just always report the full pattern to simplify the logic here... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
It is indeed a lot of code just to improve the reported location. If it would be better to simplify, I can modify this PR. |
I think the intention to report the exact location is because this rule, unlike other regex/string rules, reports multiple errors for the same node #11392 (comment) |
LGTM, but I also am thinking better to report whole the target node in order to simplify. Reporting the first capture group looks good enough. |
If all agree I can change the code to report:
|
👍 |
I would like all errors reported for better UX, but if that's a pain to implement, I'm okay with 1 error. And I definitely think the whole node should be reported. |
This shouldn't be a problem technically. If it's okay to report multiple errors from the same rule for the same location (the full node), it can be done. |
I have no problem with it since the error message lists the unnamed capture group in question. So although the location is the same, users should understand each as a distinct error. If other team members think this could be a confusing UX, I'm okay with one error. And if the error message did not show the actual issue, then I would agree we should only show one error, because multiple identical-looking errors is a bad user experience. EDIT: There's nothing which forbids multiple errors on the same node from a technical perspective. |
It's modified now. The rule still reports multiple errors for the same regular expression, but always the full node (regex Literal, NewExpression or CallExpression) as the location. |
What is the purpose of this pull request? (put an "X" next to item)
[X] Bug fix #12233
This PR fixes reporting incorrect location in
prefer-named-capture-group
when the pattern inRegExp()
ornew RegExp()
is any of the following:Tell us about your environment
What parser (default, Babel-ESLint, etc.) are you using?
default
Please show your full configuration:
Configuration
What did you do? Please include the actual source code causing the issue.
Demo link
What did you expect to happen?
3 errors with correct locations.
What actually happened? Please include the actual, raw output from ESLint.
What changes did you make? (Give an overview)
In the above cases, report the whole
arguments[0]
location.Is there anything you'd like reviewers to focus on?