no-invalid-template-strings: fix for escaped template expressions #3116
Conversation
|
|
| @@ -62,10 +62,12 @@ export class Rule extends Lint.Rules.OptionallyTypedRule { | |||
| public static FAILURE_STRING_ALPHABETICAL(name: string): string { | |||
| return `The key '${name}' is not sorted alphabetically`; | |||
| } | |||
| /* tslint:disable:no-shadowed-variable */ | |||
There was a problem hiding this comment.
sorry, master was broken, please revert these lines and merge master with #3118
|
nevermind, I did the merge |
| * Support for ignoring case: '\${template-expression}' | ||
| */ | ||
| const unescapedText: string = node.getFullText(); | ||
| const preceedingCharacter = unescapedText.substr(unescapedText.search(/\$\{/) - 1, 1); |
There was a problem hiding this comment.
this regex should be saved as a constant at the top of this function
| /** | ||
| * Support for ignoring case: '\${template-expression}' | ||
| */ | ||
| const unescapedText: string = node.getFullText(); |
There was a problem hiding this comment.
is the : string typedef necessary?
| */ | ||
| const unescapedText: string = node.getFullText(); | ||
| const preceedingCharacter = unescapedText.substr(unescapedText.search(/\$\{/) - 1, 1); | ||
| if (preceedingCharacter === "\\") { |
There was a problem hiding this comment.
I'm confused, how does this work for L1 and L3 of the test.ts.lint file you edited?
There was a problem hiding this comment.
Because the characters preceeding the lines' ${} expressions are backslashes. "\" is an escaped backslash. Could be made clearer somehow probably.
…ed to const at top of method
|
Build error is from 8000ms timeout |
| /** | ||
| * Support for ignoring case: '\${template-expression}' | ||
| */ | ||
| const unescapedText = node.getFullText(); |
There was a problem hiding this comment.
getFullText() returns the text including leading comments and whitespace. Use getText(sourceFile) instead.
| /** | ||
| * Finds instances of '${' | ||
| */ | ||
| const findTemplateString = new RegExp(/\$\{/); |
There was a problem hiding this comment.
why use new RegExp instead of an regex literal?
| /** | ||
| * Finds instances of '${' | ||
| */ | ||
| const findTemplateString = new RegExp(/\$\{/); |
| const findTemplateString = new RegExp(/\$\{/); | ||
|
|
||
| const index = node.text.search(findTemplateString); | ||
| if (index !== -1) { |
There was a problem hiding this comment.
this needs to be a loop. otherwise you only check the first occurence and don't look further
this one should result in an error: ``"${foo}bar${baz}"
| */ | ||
| const unescapedText = node.getFullText(); | ||
| const preceedingCharacter = unescapedText.substr(unescapedText.search(findTemplateString) - 1, 1); | ||
| if (isBackslash(preceedingCharacter)) { |
There was a problem hiding this comment.
you also need to check if the backslash is escaped with a backslash.
this one should result in an error: "\\${foo}"
|
ah, sorry about that. I filed #3128 to track that your comments get addressed before the next release |
PR checklist
Changelog entry
[bugfix]
no-invalid-template-stringsallows backslash-prefixed template expressions