-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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: no-regex-spaces false positives and invalid autofix (fixes #12226) #12231
Conversation
Marking as accepted as the bug
@platinumazure there is a TODO in the code, I'm not sure is it already fixed in #7053? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TODO can be removed. Thanks!
It looks like the rule is intentionally only reporting one violation and relying on multipass autofix to find multiple violations, is that correct?
Otherwise LGTM, thanks!
lib/rules/no-regex-spaces.js
Outdated
} | ||
|
||
/* | ||
* TODO: (platinumazure) Fix message to use rule message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, this TODO can be removed. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO is removed now :)
Didn't want to change that, I think it was intentional because the messages can't be more descriptive and the whole range of the node (literal/call/new) is reported, so it could be confusing to see multiple similar messages for the same node. |
Thanks @mdjermanovic. I think it would be awesome to try to report smaller ranges as a future enhancement. I'll re-review shortly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
What is the purpose of this pull request? (put an "X" next to item)
[X] Bug fix #12226 2 other bugs.
This PR fixes 3 unrelated bugs in
no-regex-spaces
:/[ ]/
fixed to/[ {2}]/
)RegExp('\\d ')
fixed toRegExp('\\ {2} ')
)RegExp('[ ')
fixed toRegExp('[ {2}')
). This might not be a bug, but I guess it's better to ignore invalid regex.1. Character classes
The following tests were failing:
The code is now using
regexpp
parser to target only spaces with"Alternative"
parent.Example:
Previous fix:
New behavior:
2. Strings with escape sequences
It was assumed that the indexes are same as in the string's source code literal presentation.
The following tests were failing:
The logic is now changed to check for consecutive spaces in the source code (
Node.raw
), and also don't fix if the raw presentation is not same as the internal.Example:
Previous fix:
New behavior:
3. Invalid syntax
The following tests were failing:
Example:
Previous fix:
New behavior:
What changes did you make? (Give an overview)
Use
regexpp
to parse, don't fix strings that have a different raw representation, don't report regex with invalid syntax.Is there anything you'd like reviewers to focus on?
Everything please, I didn't use
regexpp
before.The cases in
2.
could be auto-fixed but that would require a utility to map indexes (that could be a future enhancement, I believe at the moment it's good enough to prevent invalid autofix).There are some edge cases where the rule will report an error, e.g.
RegExp('[ ]\\u0020\\u0020')
, but that would require the same utility.