Skip to content

Update: WIP: add ignoreCharClasses option for no-useless-escape#7455

Closed
not-an-aardvark wants to merge 11 commits intomasterfrom
no-useless-escape-ignore-character-classes
Closed

Update: WIP: add ignoreCharClasses option for no-useless-escape#7455
not-an-aardvark wants to merge 11 commits intomasterfrom
no-useless-escape-ignore-character-classes

Conversation

@not-an-aardvark
Copy link
Copy Markdown
Member

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

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

What rule do you want to change?

no-useless-escape

Does this change cause the rule to produce more or fewer warnings?

Fewer, by opt-in

How will the change be implemented? (New option, new default behavior, etc.)?

A new option will be added.

Please provide some example code that this change will affect:

/* eslint no-useless-escape: [2, {"ignoreCharClasses": true}] */

var foo = /[\.\*]/; // a character class matching a dot or a star character

What does the rule currently do for this code?

It reports the escapes as useless, since they can be removed without affecting the code.

What will the rule do after it's changed?

With the new ignoreCharClasses: true option, the rule will allow characters in regular expressions to be uselessly escaped, if that character is sometimes a special character in regular expressions. For example, with the new option enabled, \* will be allowed in all regular expressions, even though escaping is only required for * in a character class. The default is ignoreCharClasses: false.

What changes did you make? (Give an overview)

This adds an ignoreCharClasses option to no-useless-escape, in case people would prefer to always escape certain characters in regular expressions rather than worrying about whether the character is in a character class.

TODO: Add documentation for the new option

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

Do not merge yet; this depends on #7425. Specifically, see #7425 (comment) for discussion about this option.

We still need to decide whether it's worth adding this option. I'm not opposed to adding it if a lot of people are interested in it, but I'm not championing it at the moment; I think maybe we should release #7425 and see how many people would actually want this option. I'm making this PR now in case we decide we want to get this change into the same release as #7425.

@not-an-aardvark not-an-aardvark added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules needs bikeshedding Minor details about this change need to be discussed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion do not merge This pull request should not be merged yet labels Oct 26, 2016
@not-an-aardvark not-an-aardvark self-assigned this Oct 26, 2016
@eslintbot
Copy link
Copy Markdown

LGTM

@mention-bot
Copy link
Copy Markdown

@not-an-aardvark, thanks for your PR! By analyzing the history of the files in this pull request, we identified @onurtemizkan, @vitorbal and @mysticatea to be potential reviewers.

@not-an-aardvark not-an-aardvark force-pushed the no-useless-escape-ignore-character-classes branch from 2f58a2d to fa5df6e Compare October 26, 2016 20:50
@eslintbot
Copy link
Copy Markdown

LGTM

@mysticatea
Copy link
Copy Markdown
Member

Thank you for the contribution!

Honestly, I don't think this option is necessary because I don't feel that the useless escapes in character classes improve readability. Granted #7425 will increase warnings, but it's definitely a bug fix.

@not-an-aardvark
Copy link
Copy Markdown
Member Author

I'm going to close this for now, because we decided to merge #7425 as-is. We can reopen it if we decide to add a new option.

@not-an-aardvark not-an-aardvark deleted the no-useless-escape-ignore-character-classes branch October 29, 2016 23:58
@eslint-deprecated eslint-deprecated Bot locked and limited conversation to collaborators Feb 6, 2018
@eslint-deprecated eslint-deprecated Bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

archived due to age This issue has been archived; please open a new issue for any further discussion do not merge This pull request should not be merged yet enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion needs bikeshedding Minor details about this change need to be discussed rule Relates to ESLint's core rules

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants