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
Change double-quote hygiene rule to tslint rule #6514
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| "use strict"; | ||
| /*--------------------------------------------------------------------------------------------- | ||
| * Copyright (c) Microsoft Corporation. All rights reserved. | ||
| * Licensed under the Source EULA. See License.txt in the project root for license information. | ||
| *--------------------------------------------------------------------------------------------*/ | ||
| Object.defineProperty(exports, "__esModule", { value: true }); | ||
| const ts = require("typescript"); | ||
| const Lint = require("tslint"); | ||
| /** | ||
| * Implementation of the double-quoted-string-arg rule which verifies that the specified index of calls matching | ||
| * the specified signatures is quoted with double-quotes only. | ||
| */ | ||
| class Rule extends Lint.Rules.AbstractRule { | ||
| apply(sourceFile) { | ||
| return this.applyWithWalker(new DoubleQuotedStringArgRuleWalker(sourceFile, this.getOptions())); | ||
| } | ||
| } | ||
| exports.Rule = Rule; | ||
| class DoubleQuotedStringArgRuleWalker extends Lint.RuleWalker { | ||
| constructor(file, opts) { | ||
| super(file, opts); | ||
| this.signatures = Object.create(null); | ||
| this.argIndex = undefined; | ||
| const options = this.getOptions(); | ||
| const first = options && options.length > 0 ? options[0] : null; | ||
| if (first) { | ||
| if (Array.isArray(first.signatures)) { | ||
| first.signatures.forEach((signature) => this.signatures[signature] = true); | ||
| } | ||
| if (typeof first.argIndex !== 'undefined') { | ||
| this.argIndex = first.argIndex; | ||
| } | ||
| } | ||
| } | ||
| visitCallExpression(node) { | ||
| this.checkCallExpression(node); | ||
| super.visitCallExpression(node); | ||
| } | ||
| checkCallExpression(node) { | ||
| // Not one of the functions we're looking for, continue on | ||
| const functionName = node.expression.getText(); | ||
| if (functionName && !this.signatures[functionName]) { | ||
| return; | ||
| } | ||
| const arg = node.arguments[this.argIndex]; | ||
| // Ignore if the arg isn't a string - we expect the compiler to warn if that's an issue | ||
| if (arg && ts.isStringLiteral(arg)) { | ||
| const argText = arg.getText(); | ||
| const doubleQuotedArg = argText.length >= 2 && argText[0] === DoubleQuotedStringArgRuleWalker.DOUBLE_QUOTE && argText[argText.length - 1] === DoubleQuotedStringArgRuleWalker.DOUBLE_QUOTE; | ||
| if (!doubleQuotedArg) { | ||
| const fix = [ | ||
| Lint.Replacement.replaceFromTo(arg.getStart(), arg.getWidth(), `"${arg.getText().slice(1, arg.getWidth() - 2)}"`), | ||
| ]; | ||
| this.addFailure(this.createFailure(arg.getStart(), arg.getWidth(), `Argument ${this.argIndex + 1} to '${functionName}' must be double quoted.`, fix)); | ||
| return; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| DoubleQuotedStringArgRuleWalker.DOUBLE_QUOTE = '"'; |
This file contains 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| /*--------------------------------------------------------------------------------------------- | ||
| * Copyright (c) Microsoft Corporation. All rights reserved. | ||
| * Licensed under the Source EULA. See License.txt in the project root for license information. | ||
| *--------------------------------------------------------------------------------------------*/ | ||
|
|
||
| import * as ts from 'typescript'; | ||
| import * as Lint from 'tslint'; | ||
|
|
||
| /** | ||
| * Implementation of the double-quoted-string-arg rule which verifies that the specified index of calls matching | ||
| * the specified signatures is quoted with double-quotes only. | ||
| */ | ||
| export class Rule extends Lint.Rules.AbstractRule { | ||
| public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { | ||
| return this.applyWithWalker(new DoubleQuotedStringArgRuleWalker(sourceFile, this.getOptions())); | ||
| } | ||
| } | ||
|
|
||
| interface Map<V> { | ||
| [key: string]: V; | ||
| } | ||
|
|
||
| interface DoubleQuotedStringArgOptions { | ||
| signatures?: string[]; | ||
| argIndex?: number; | ||
| } | ||
|
|
||
| class DoubleQuotedStringArgRuleWalker extends Lint.RuleWalker { | ||
|
|
||
| private static DOUBLE_QUOTE: string = '"'; | ||
|
|
||
| private signatures: Map<boolean>; | ||
| private argIndex: number | undefined; | ||
|
|
||
| constructor(file: ts.SourceFile, opts: Lint.IOptions) { | ||
| super(file, opts); | ||
| this.signatures = Object.create(null); | ||
| this.argIndex = undefined; | ||
| const options: any[] = this.getOptions(); | ||
| const first: DoubleQuotedStringArgOptions = options && options.length > 0 ? options[0] : null; | ||
| if (first) { | ||
| if (Array.isArray(first.signatures)) { | ||
| first.signatures.forEach((signature: string) => this.signatures[signature] = true); | ||
| } | ||
| if (typeof first.argIndex !== 'undefined') { | ||
| this.argIndex = first.argIndex; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| protected visitCallExpression(node: ts.CallExpression): void { | ||
| this.checkCallExpression(node); | ||
| super.visitCallExpression(node); | ||
| } | ||
|
|
||
| private checkCallExpression(node: ts.CallExpression): void { | ||
| // Not one of the functions we're looking for, continue on | ||
| const functionName = node.expression.getText(); | ||
| if (functionName && !this.signatures[functionName]) { | ||
| return; | ||
| } | ||
|
|
||
| const arg = node.arguments[this.argIndex!]; | ||
|
|
||
| // Ignore if the arg isn't a string - we expect the compiler to warn if that's an issue | ||
| if(arg && ts.isStringLiteral(arg)) { | ||
| const argText = arg.getText(); | ||
| const doubleQuotedArg = argText.length >= 2 && argText[0] === DoubleQuotedStringArgRuleWalker.DOUBLE_QUOTE && argText[argText.length - 1] === DoubleQuotedStringArgRuleWalker.DOUBLE_QUOTE; | ||
|
|
||
| if (!doubleQuotedArg) { | ||
| const fix = [ | ||
| Lint.Replacement.replaceFromTo(arg.getStart(), arg.getWidth(), `"${arg.getText().slice(1, arg.getWidth() - 2)}"`), | ||
| ]; | ||
| this.addFailure(this.createFailure( | ||
| arg.getStart(), arg.getWidth(), | ||
| `Argument ${this.argIndex! + 1} to '${functionName}' must be double quoted.`, fix)); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| } | ||
| } |
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
Oops, something went wrong.
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.
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.
Youy don't need to add this to the hygiene run, this will automatically run in the tslint check if you have the rule in the tslint directory and have the rule in the tslint file.
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.
If your goal is to limit the scope of the files included for this rule, I would just modify the tslint gulp task to run a second pass with just our lint file, rather than putting this into the hygiene task.
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 could also see if vscode has interest in taking this rule, since they have the guidance and when they change code they general update any code that doesn't follow the guidance, they probably just don't see this as big enough of a problem to dedicate time to fix.
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.
So I didn't add it to the tslint.json because that runs on all files and I didn't want to fix up the vscode ones since that'd just result in a bunch of changes that might cause issues merging down the line. Hence the creation of the new tslint file that will run on just the sql code. (I'll update it later to run on our extensions too - I just didn't want to do the changes now since the fix logic isn't working for some reason so I need to investigate that further and doing the replacements manually is a pain)
And I still want it to run as part of hygiene so that the precommit hook catches it - since my current understanding if I'm reading the configuration correctly is that it only runs the hygiene task as a precommit hook and thus if someone ignores the GCI failing (which people do quite a bit) they might not catch the issue.
I'll follow up with seeing if vscode will take this but for now I figure we can get it in our repo and then remove it later if they decide to take it.