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

Fix: prefer-named-capture-group incorrect locations (fixes #12233) #12247

Merged
merged 2 commits into from Sep 29, 2019

Conversation

@mdjermanovic
Copy link
Member

mdjermanovic commented Sep 9, 2019

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 in RegExp() or new RegExp() is any of the following:

  • Computed string
  • String with an escape sequence
  • Multiline string

Tell us about your environment

  • ESLint Version: 6.3.0
  • Node Version: 10.16.0
  • npm Version: 6.9.0

What parser (default, Babel-ESLint, etc.) are you using?

default

Please show your full configuration:

Configuration
module.exports = {
  parserOptions: {
    ecmaVersion: 2015,
  }
};

What did you do? Please include the actual source code causing the issue.

Demo link

/* eslint prefer-named-capture-group:error */

RegExp('' + '(a)');
RegExp('\\d\\d(a)');
RegExp(`
(a)`
);

What did you expect to happen?

3 errors with correct locations.

What actually happened? Please include the actual, raw output from ESLint.

  3:9   error  Capture group '(a)' should be converted to a named or non-capturing group  prefer-named-capture-group
  4:13  error  Capture group '(a)' should be converted to a named or non-capturing group  prefer-named-capture-group
  5:10  error  Capture group '(a)' should be converted to a named or non-capturing group  prefer-named-capture-group

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?

@eslint eslint bot added the triage label Sep 9, 2019
@mdjermanovic mdjermanovic added accepted bug rule and removed triage labels Sep 9, 2019
@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented Sep 9, 2019

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...

Copy link
Member

platinumazure left a comment

LGTM, thanks!

@mdjermanovic

This comment has been minimized.

Copy link
Member Author

mdjermanovic commented Sep 9, 2019

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.

@mdjermanovic

This comment has been minimized.

Copy link
Member Author

mdjermanovic commented Sep 11, 2019

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)

@mysticatea

This comment has been minimized.

Copy link
Member

mysticatea commented Sep 13, 2019

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.

@mdjermanovic

This comment has been minimized.

Copy link
Member Author

mdjermanovic commented Sep 13, 2019

If all agree I can change the code to report:

  • Just 1 error (the first one), with its group content in the message.
  • The whole new RegExp/regex literal node as location.
@mysticatea

This comment has been minimized.

Copy link
Member

mysticatea commented Sep 13, 2019

👍

@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented Sep 14, 2019

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.

@mdjermanovic

This comment has been minimized.

Copy link
Member Author

mdjermanovic commented Sep 14, 2019

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.

@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented Sep 14, 2019

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.

@mdjermanovic

This comment has been minimized.

Copy link
Member Author

mdjermanovic commented Sep 16, 2019

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.

@platinumazure platinumazure requested a review from mysticatea Sep 29, 2019
@mysticatea mysticatea merged commit b349bf7 into master Sep 29, 2019
16 checks passed
16 checks passed
Verify Files
Details
Test (ubuntu-latest, 8.x)
Details
Test (ubuntu-latest, 10.x)
Details
Test (ubuntu-latest, 12.x)
Details
Test (windows-latest, 12.x)
Details
Test (macOS-latest, 12.x)
Details
Browser Test
Details
commit-message PR title follows commit message guidelines
Details
continuous-integration Build #20190916.4 succeeded
Details
continuous-integration (Test on Node.js 10 (Linux)) Test on Node.js 10 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Linux)) Test on Node.js 12 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Windows)) Test on Node.js 12 (Windows) succeeded
Details
continuous-integration (Test on Node.js 12 (macOS)) Test on Node.js 12 (macOS) succeeded
Details
continuous-integration (Test on Node.js 8 (Linux)) Test on Node.js 8 (Linux) succeeded
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
@mysticatea mysticatea deleted the issue12233 branch Sep 29, 2019
@eslint eslint bot locked and limited conversation to collaborators Mar 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.