Update: WIP: add ignoreCharClasses option for no-useless-escape#7455
Closed
not-an-aardvark wants to merge 11 commits intomasterfrom
Closed
Update: WIP: add ignoreCharClasses option for no-useless-escape#7455not-an-aardvark wants to merge 11 commits intomasterfrom
ignoreCharClasses option for no-useless-escape#7455not-an-aardvark wants to merge 11 commits intomasterfrom
Conversation
|
LGTM |
|
@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. |
2f58a2d to
fa5df6e
Compare
|
LGTM |
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. |
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. |
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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-escapeDoes 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:
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: trueoption, 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 isignoreCharClasses: false.What changes did you make? (Give an overview)
This adds an
ignoreCharClassesoption tono-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.