Add prefer-replace-all rule
#488
Conversation
rules/prefer-replace-all.js
Outdated
|
|
||
| function checkGlobalFlag(node) { | ||
| const searchPattern = node.arguments[0]; | ||
| return Object.prototype.hasOwnProperty.call(searchPattern, 'regex') && searchPattern.regex.flags === 'g'; |
There was a problem hiding this comment.
why not just searchPattern && searchPattern.regex && searchPattern.regex.flags === 'g'.
I understand flags with i should not pass, but how about other flags like gu?
There was a problem hiding this comment.
Does the built in replace method have full unicode support? From what I'm reading it seems like it doesn't. And all of the other flags would change the search behavior as well.
rules/prefer-replace-all.js
Outdated
|
|
||
| function replaceNode(node, fixer) { | ||
| const stringName = node.callee.object.name; | ||
| const searchPattern = node.arguments[0].regex.pattern.replace(/\\(.)/g, '$1'); |
There was a problem hiding this comment.
This line is not right, /\\a/g is searching for \a not a, maybe .replace(/\\\\(.)/g, '\\$1')?.
Also, funny thing.
I think we can also replace it with .replace(/\\\\/g, '\\'), then this line will against this rule itself, so maybe .replaceAll('\\\\', '\\')?
There was a problem hiding this comment.
Hmm. On second thought, that replace shouldn't be necessary at all. However, I do need to add an escape for any double quotes in the original regex expression.
Yeah, that ocurred to me while I was writing it. Very ironic. I decided not to use replaceAll since it doesn't have support in node yet and it would be transpiled back to the expression I have if we ran it through babel.
|
@jmoore914 Can you check 715eb67 ? Make sure I didn't miss any test cases |
test/prefer-replace-all.js
Outdated
| // Escaped symbols | ||
| { | ||
| code: 'foo.replace(/searchPattern/g, \'\\\'escapedQuotes\\\'\')', | ||
| output: 'foo.replaceAll(\'searchPattern\', \'\\\'escapedQuotes\\\'\')', | ||
| code: 'foo.replace(/\\./g, bar)', | ||
| output: 'foo.replaceAll(\'.\', bar)', | ||
| errors: [error] | ||
| }, |
There was a problem hiding this comment.
@jmoore914 I think I found a broken case, we still need replace \
There was a problem hiding this comment.
Maybe it's not that simple at all, I think we should test all these cases
/\./g
/\\./g
/\\\./g
Co-Authored-By: fisker Cheung <lionkay@gmail.com>
Newest PR should fix this comment |
|
Thanks :) |
Fixes #433