From d4b50a0824cd0b39c98b7afd0f815122f40ee889 Mon Sep 17 00:00:00 2001 From: Evan SUAU Date: Mon, 5 Nov 2018 02:32:29 +0100 Subject: [PATCH] Add regex option to no-suspicious-comment rule (#631) * Fix Microsoft/tslint-microsoft-contrib#440: Add regex option to no-suspicious-comment * Fix Microsoft/tslint-microsoft-contrib#440 Mention regex in failure message * Microsoft/tslint-microsoft-contrib#631 Mark 'exceptionRegex' as readonly --- src/noSuspiciousCommentRule.ts | 52 ++++++++++++++++++++--- src/tests/NoSuspiciousCommentRuleTests.ts | 44 +++++++++++++++++++ 2 files changed, 91 insertions(+), 5 deletions(-) diff --git a/src/noSuspiciousCommentRule.ts b/src/noSuspiciousCommentRule.ts index 0741d18c1..dc0f28230 100644 --- a/src/noSuspiciousCommentRule.ts +++ b/src/noSuspiciousCommentRule.ts @@ -1,11 +1,12 @@ import * as ts from 'typescript'; import * as Lint from 'tslint'; -import {forEachTokenWithTrivia} from 'tsutils'; -import {ExtendedMetadata} from './utils/ExtendedMetadata'; +import { forEachTokenWithTrivia } from 'tsutils'; +import { ExtendedMetadata } from './utils/ExtendedMetadata'; const FAILURE_STRING: string = 'Suspicious comment found: '; const SUSPICIOUS_WORDS = ['BUG', 'HACK', 'FIXME', 'LATER', 'LATER2', 'TODO']; +const FAILURE_STRING_OPTION: string = '\nTo disable this warning, the comment should include one of the following regex: '; export class Rule extends Lint.Rules.AbstractRule { @@ -13,8 +14,15 @@ export class Rule extends Lint.Rules.AbstractRule { ruleName: 'no-suspicious-comment', type: 'maintainability', description: `Do not use suspicious comments, such as ${SUSPICIOUS_WORDS.join(', ')}`, - options: null, // tslint:disable-line:no-null-keyword - optionsDescription: '', + options: { + type: 'array', + items: { + type: 'string' + } + }, + optionsDescription: `One argument may be optionally provided: \n\n' + + '* an array of regex that disable the warning if one or several of them + are found in the comment text. (Example: \`['https://github.com/my-org/my-project/*']\`)`, typescriptOnly: true, issueClass: 'Non-SDL', issueType: 'Warning', @@ -31,6 +39,17 @@ export class Rule extends Lint.Rules.AbstractRule { class NoSuspiciousCommentRuleWalker extends Lint.RuleWalker { + private readonly exceptionRegex: RegExp[] = []; + + constructor(sourceFile: ts.SourceFile, options: Lint.IOptions) { + super(sourceFile, options); + if (options.ruleArguments !== undefined && options.ruleArguments.length > 0) { + options.ruleArguments.forEach((regexStr: string) => { + this.exceptionRegex.push(new RegExp(regexStr)); + }); + } + } + public visitSourceFile(node: ts.SourceFile) { forEachTokenWithTrivia(node, (text, tokenSyntaxKind, range) => { if (tokenSyntaxKind === ts.SyntaxKind.SingleLineCommentTrivia || @@ -41,6 +60,9 @@ class NoSuspiciousCommentRuleWalker extends Lint.RuleWalker { } private scanCommentForSuspiciousWords(startPosition: number, commentText: string): void { + if (this.commentContainsExceptionRegex(this.exceptionRegex, commentText)) { + return; + } SUSPICIOUS_WORDS.forEach((suspiciousWord: string) => { this.scanCommentForSuspiciousWord(suspiciousWord, commentText, startPosition); }); @@ -55,7 +77,27 @@ class NoSuspiciousCommentRuleWalker extends Lint.RuleWalker { } private foundSuspiciousComment(startPosition: number, commentText: string, suspiciousWord: string) { - const errorMessage: string = FAILURE_STRING + suspiciousWord; + let errorMessage: string = FAILURE_STRING + suspiciousWord; + if (this.exceptionRegex.length > 0) { + errorMessage += '.' + this.getFailureMessageWithExceptionRegexOption(); + } this.addFailureAt(startPosition, commentText.length, errorMessage); } + + private commentContainsExceptionRegex(exceptionRegex: RegExp[], commentText: string): boolean { + for (const regex of exceptionRegex) { + if (regex.test(commentText)) { + return true; + } + } + return false; + } + + private getFailureMessageWithExceptionRegexOption(): string { + let message: string = FAILURE_STRING_OPTION; + this.exceptionRegex.forEach((regex: RegExp) => { + message += regex.toString(); + }); + return message; + } } diff --git a/src/tests/NoSuspiciousCommentRuleTests.ts b/src/tests/NoSuspiciousCommentRuleTests.ts index 8578bf9db..531ea437a 100644 --- a/src/tests/NoSuspiciousCommentRuleTests.ts +++ b/src/tests/NoSuspiciousCommentRuleTests.ts @@ -7,6 +7,7 @@ import {TestHelper} from './TestHelper'; describe('noSuspiciousCommentRule', (): void => { const ruleName: string = 'no-suspicious-comment'; + const option: string[] = ['https://example.com/*']; it('should pass on normal comments', (): void => { const script: string = ` @@ -14,6 +15,7 @@ describe('noSuspiciousCommentRule', (): void => { `; TestHelper.assertViolations(ruleName, script, []); + TestHelper.assertViolationsWithOptions(ruleName, option, script, []); }); it('should pass on multi-line comments', (): void => { @@ -53,6 +55,17 @@ describe('noSuspiciousCommentRule', (): void => { }]); }); + it('should pass on multiline TODO comments with regex option', (): void => { + const script: string = ` + /** + * TODO: add failing example and update assertions + * See https://example.com/article/123 + */ + `; + + TestHelper.assertViolationsWithOptions(ruleName, option, script, []); + }); + it('should pass on lower case todo comments without colons', (): void => { const script: string = ` // todo add failing example and update assertions @@ -76,6 +89,21 @@ describe('noSuspiciousCommentRule', (): void => { "ruleName": "no-suspicious-comment", "startPosition": {"character": 17, "line": 2} }]); + TestHelper.assertViolationsWithOptions(ruleName, option, script, [{ + "failure": `Suspicious comment found: ${suspiciousWord}. +To disable this warning, the comment should include one of the following regex: /https:\\/\\/example.com\\/*/`, + "name": Utils.absolutePath("file.ts"), + "ruleName": "no-suspicious-comment", + "startPosition": {"character": 17, "line": 2} + }]); + }); + + it(`should pass on upper case ${suspiciousWord} comments without colons and with regex option`, (): void => { + const script: string = ` + // ${suspiciousWord} you should fix this https://example.com/article/123 + `; + + TestHelper.assertViolationsWithOptions(ruleName, option, script, []); }); it(`should fail on upper case ${suspiciousWord} comments with colons`, (): void => { @@ -91,6 +119,14 @@ describe('noSuspiciousCommentRule', (): void => { }]); }); + it(`should pass on upper case ${suspiciousWord} comments with colons and with regex option`, (): void => { + const script: string = ` + // ${suspiciousWord}: you should fix this (see https://example.com/article/123) + `; + + TestHelper.assertViolationsWithOptions(ruleName, option, script, []); + }); + it(`should fail on lower case ${suspiciousWord} comments with colons`, (): void => { const script: string = ` // ${suspiciousWord}: you should fix this @@ -103,6 +139,14 @@ describe('noSuspiciousCommentRule', (): void => { "startPosition": {"character": 17, "line": 2} }]); }); + + it(`should pass on lower case ${suspiciousWord} comments with colons and with regex option`, (): void => { + const script: string = ` + // ${suspiciousWord}: you should fix this (see https://example.com/article/123) + `; + + TestHelper.assertViolationsWithOptions(ruleName, option, script, []); + }); }); }); /* tslint:enable:quotemark */