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
Merged

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

merged 2 commits into from Dec 20, 2021

Conversation

SebastianSimon
Copy link
Contributor

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 bot commented Dec 12, 2021

CLA Signed

The committers are authorized under a signed CLA.

@eslint-github-bot eslint-github-bot bot added triage An ESLint team member will look at this issue soon documentation Relates to ESLint's documentation labels Dec 12, 2021
@mdjermanovic mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Dec 13, 2021
@@ -26,7 +26,8 @@ Examples of **incorrect** code for this rule:
`\#{foo}`;
/\!/;
/\@/;

/[\[\]]/;
Copy link
Member

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

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

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

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

Choose a reason for hiding this comment

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

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
@SebastianSimon SebastianSimon deleted the patch-1 branch December 20, 2021 11:00
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Jun 19, 2022
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jun 19, 2022
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 documentation Relates to ESLint's documentation rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants