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

docs: Mention character classes in no-useless-escape #15421

Merged
merged 2 commits into from Dec 20, 2021

Conversation

SebastianSimon
Copy link
Contributor

@SebastianSimon SebastianSimon commented Dec 12, 2021

Prerequisites checklist

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

[X] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

In regex literals, /[\[]/ and /[a\-]/ contain useless escapes within character classes, but these cases are not very well-known, so I’ve included them. A recent Stack Overflow question (https://stackoverflow.com/q/70322972/4642212) prompted this PR, but of course there’s also issue #13826 right here: getting a linter warning for these cases may be confusing. Therefore, I’ve explicitly mentioned this in the documentation. Note: this behavior is already part of the tests.

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

Nothing specific.

@linux-foundation-easycla
Copy link

@linux-foundation-easycla linux-foundation-easycla bot commented Dec 12, 2021

CLA Signed

The committers are authorized under a signed CLA.

@eslint-github-bot eslint-github-bot bot added triage documentation labels Dec 12, 2021
@mdjermanovic mdjermanovic added accepted rule and removed triage labels Dec 13, 2021
@@ -26,7 +26,8 @@ Examples of **incorrect** code for this rule:
`\#{foo}`;
/\!/;
/\@/;
/[\[\]]/;
Copy link
Member

@mdjermanovic mdjermanovic Dec 13, 2021

Choose a reason for hiding this comment

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

This example might be misleading because only one of the two escapes is useless, so it might be better to remove the other from the example?

Suggested change
/[\[\]]/;
/[\[]/;

Copy link
Contributor Author

@SebastianSimon SebastianSimon Dec 14, 2021

Choose a reason for hiding this comment

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

Should the “correct” example also be adjusted? When the “incorrect” and “correct” examples are compared, it’s clear which escape is useless. I was kind of trying to include realistic examples and I’m not sure if /[\[]/ qualifies. Still, not sure what the best set of examples is.

Copy link
Member

@mdjermanovic mdjermanovic Dec 14, 2021

Choose a reason for hiding this comment

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

We can add to “correct” examples both /[[]/; to mirror the incorrect /[\[]/;, and /[[\]]/; to show that \] isn't useless.

Copy link
Contributor Author

@SebastianSimon SebastianSimon Dec 18, 2021

Choose a reason for hiding this comment

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

It looks a bit redundant this way. I think it looks good if in “incorrect”, there’s the example /[\[]/;, and in “correct”, there’s the two examples /[[]/; and /[\]]/.

(By the way, sorry for the delay. This week was busy…)

Copy link
Member

@mdjermanovic mdjermanovic left a comment

LGTM, thanks for contributing!

@mdjermanovic mdjermanovic changed the title docs: no-useless-escape: Mention character classes docs: Mention character classes in no-useless-escape Dec 20, 2021
@mdjermanovic mdjermanovic merged commit 775d181 into eslint:main Dec 20, 2021
13 of 14 checks passed
@SebastianSimon SebastianSimon deleted the patch-1 branch Dec 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted documentation rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants