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

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-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Sep 9, 2019
@mdjermanovic mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Sep 9, 2019
@platinumazure
Copy link
Member

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

@mdjermanovic
Copy link
Member Author

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

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

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

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

👍

@platinumazure
Copy link
Member

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

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

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.

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 bug ESLint is working incorrectly rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

prefer-named-capture-group is reporting incorrect locations
5 participants