From 7fc3d6249b181473f1c6bf7a4e4c5558e595c80f Mon Sep 17 00:00:00 2001 From: Klaus Meinhardt Date: Wed, 31 May 2017 20:56:24 +0200 Subject: [PATCH] Rewrite and simplify comment-format (#2616) --- src/enableDisableRules.ts | 16 +- src/rules/commentFormatRule.ts | 206 ++++++++---------- .../exceptions-pattern/test.ts.lint | 5 +- .../exceptions-words/test.ts.lint | 4 +- test/rules/comment-format/lower/test.ts.lint | 4 +- test/rules/comment-format/upper/test.ts.lint | 5 +- 6 files changed, 110 insertions(+), 130 deletions(-) diff --git a/src/enableDisableRules.ts b/src/enableDisableRules.ts index fd653840fa8..d2e54c4ce93 100644 --- a/src/enableDisableRules.ts +++ b/src/enableDisableRules.ts @@ -21,6 +21,15 @@ import * as utils from "tsutils"; import * as ts from "typescript"; import { RuleFailure } from "./language/rule/rule"; +/** + * regex is: start of string followed by any amount of whitespace + * followed by tslint and colon + * followed by either "enable" or "disable" + * followed optionally by -line or -next-line + * followed by either colon, whitespace or end of string + */ +export const ENABLE_DISABLE_REGEX = /^\s*tslint:(enable|disable)(?:-(line|next-line))?(:|\s|$)/; + export function removeDisabledFailures(sourceFile: ts.SourceFile, failures: RuleFailure[]): RuleFailure[] { if (failures.length === 0) { // Usually there won't be failures anyway, so no need to look for "tslint:disable". @@ -125,12 +134,7 @@ function getSwitchRange(modifier: Modifier, range: ts.TextRange, sourceFile: ts. type Modifier = "line" | "next-line" | undefined; function parseComment(commentText: string): { rulesList: string[] | "all"; isEnabled: boolean; modifier: Modifier } | undefined { - // regex is: start of string followed by any amount of whitespace - // followed by tslint and colon - // followed by either "enable" or "disable" - // followed optionally by -line or -next-line - // followed by either colon, whitespace or end of string - const match = /^\s*tslint:(enable|disable)(?:-(line|next-line))?(:|\s|$)/.exec(commentText); + const match = ENABLE_DISABLE_REGEX.exec(commentText); if (match === null) { return undefined; } diff --git a/src/rules/commentFormatRule.ts b/src/rules/commentFormatRule.ts index da11ced2153..35f1c5f90b9 100644 --- a/src/rules/commentFormatRule.ts +++ b/src/rules/commentFormatRule.ts @@ -18,15 +18,27 @@ import * as utils from "tsutils"; import * as ts from "typescript"; +import { ENABLE_DISABLE_REGEX } from "../enableDisableRules"; import * as Lint from "../index"; -import { escapeRegExp } from "../utils"; +import { escapeRegExp, isLowerCase, isUpperCase } from "../utils"; interface IExceptionsObject { "ignore-words"?: string[]; "ignore-pattern"?: string; } -type ExceptionsRegExp = RegExp | null; +interface Options { + space: boolean; + case: Case; + exceptions?: RegExp; + failureSuffix: string; +} + +const enum Case { + None, + Lower, + Upper, +} const OPTION_SPACE = "check-space"; const OPTION_LOWERCASE = "check-lowercase"; @@ -42,7 +54,8 @@ export class Rule extends Lint.Rules.AbstractRule { Three arguments may be optionally provided: * \`"check-space"\` requires that all single-line comments must begin with a space, as in \`// comment\` - * note that comments starting with \`///\` are also allowed, for things such as \`///\` + * note that for comments starting with multiple slashes, e.g. \`///\`, leading slashes are ignored + * TypeScript reference comments are ignored completely * \`"check-lowercase"\` requires that the first non-whitespace character of a comment must be lowercase, if applicable. * \`"check-uppercase"\` requires that the first non-whitespace character of a comment must be uppercase, if applicable. @@ -103,136 +116,89 @@ export class Rule extends Lint.Rules.AbstractRule { public static IGNORE_PATTERN_FAILURE_FACTORY = (pattern: string): string => ` or its start must match the regex pattern "${pattern}"`; public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { - return this.applyWithWalker(new CommentWalker(sourceFile, this.getOptions())); + return this.applyWithFunction(sourceFile, walk, parseOptions(this.ruleArguments)); } } -class CommentWalker extends Lint.RuleWalker { - private exceptionsRegExp: ExceptionsRegExp; - private failureIgnorePart = ""; - - constructor(sourceFile: ts.SourceFile, options: Lint.IOptions) { - super(sourceFile, options); +function parseOptions(options: Array): Options { + return { + case: options.indexOf(OPTION_LOWERCASE) !== -1 + ? Case.Lower + : options.indexOf(OPTION_UPPERCASE) !== -1 + ? Case.Upper + : Case.None, + failureSuffix: "", + space: options.indexOf(OPTION_SPACE) !== -1, + ...composeExceptions(options[options.length - 1]), + }; +} - this.exceptionsRegExp = this.composeExceptionsRegExp(); +function composeExceptions(option?: string | IExceptionsObject): undefined | {exceptions: RegExp; failureSuffix: string} { + if (typeof option !== "object") { + return undefined; } - - public visitSourceFile(node: ts.SourceFile) { - utils.forEachComment(node, (fullText, comment) => { - if (comment.kind === ts.SyntaxKind.SingleLineCommentTrivia) { - const commentText = fullText.substring(comment.pos, comment.end); - const startPosition = comment.pos + 2; - const width = commentText.length - 2; - if (this.hasOption(OPTION_SPACE)) { - if (!startsWithSpace(commentText)) { - this.addFailureAt(startPosition, width, Rule.LEADING_SPACE_FAILURE); - } - } - if (this.hasOption(OPTION_LOWERCASE)) { - if (!startsWithLowercase(commentText) && !this.startsWithException(commentText)) { - this.addFailureAt(startPosition, width, Rule.LOWERCASE_FAILURE + this.failureIgnorePart); - } - } - if (this.hasOption(OPTION_UPPERCASE)) { - if (!startsWithUppercase(commentText) && !isEnableDisableFlag(commentText) && !this.startsWithException(commentText)) { - this.addFailureAt(startPosition, width, Rule.UPPERCASE_FAILURE + this.failureIgnorePart); - } - } - } - }); + const ignorePattern = option["ignore-pattern"]; + if (ignorePattern !== undefined) { + return { + exceptions: new RegExp(`^\\s*(${ignorePattern})`), + failureSuffix: Rule.IGNORE_PATTERN_FAILURE_FACTORY(ignorePattern), + }; } - private startsWithException(commentText: string): boolean { - if (this.exceptionsRegExp == null) { - return false; - } - - return this.exceptionsRegExp.test(commentText); + const ignoreWords = option["ignore-words"]; + if (ignoreWords !== undefined && ignoreWords.length !== 0) { + return { + exceptions: new RegExp(`^\\s*(?:${ignoreWords.map((word) => escapeRegExp(word.trim())).join("|")})\\b`), + failureSuffix: Rule.IGNORE_WORDS_FAILURE_FACTORY(ignoreWords), + }; } + return undefined; +} - private composeExceptionsRegExp(): ExceptionsRegExp { - const optionsList = this.getOptions() as Array; - const exceptionsObject = optionsList[optionsList.length - 1]; - - // early return if last element is string instead of exceptions object - if (typeof exceptionsObject === "string" || exceptionsObject === undefined) { - return null; +function walk(ctx: Lint.WalkContext) { + utils.forEachComment(ctx.sourceFile, (fullText, {kind, pos, end}) => { + let start = pos + 2; + if (kind !== ts.SyntaxKind.SingleLineCommentTrivia || + // exclude empty comments + start === end || + // exclude /// + fullText[start] === "/" && ctx.sourceFile.referencedFiles.some((ref) => ref.pos >= pos && ref.end <= end)) { + return; } - - if (exceptionsObject["ignore-pattern"] !== undefined) { - const ignorePattern = exceptionsObject["ignore-pattern"] as string; - this.failureIgnorePart = Rule.IGNORE_PATTERN_FAILURE_FACTORY(ignorePattern); - // regex is "start of string"//"any amount of whitespace" followed by user provided ignore pattern - return new RegExp(`^//\\s*(${ignorePattern})`); + // skip all leading slashes + while (fullText[start] === "/") { + ++start; } - - if (exceptionsObject["ignore-words"] !== undefined) { - const ignoreWords = exceptionsObject["ignore-words"] as string[]; - this.failureIgnorePart = Rule.IGNORE_WORDS_FAILURE_FACTORY(ignoreWords); - // Converts all exceptions values to strings, trim whitespace, escapes RegExp special characters and combines into alternation - const wordsPattern = ignoreWords - .map(String) - .map((str) => str.trim()) - .map(escapeRegExp) - .join("|"); - - // regex is "start of string"//"any amount of whitespace"("any word from ignore list") followed by non alphanumeric character - return new RegExp(`^//\\s*(${wordsPattern})\\b`); + if (start === end) { + return; + } + const commentText = fullText.slice(start, end); + // whitelist //#region and //#endregion and JetBrains IDEs' "//noinspection ..." + if (/^(?:#(?:end)?region|noinspection\s)/.test(commentText)) { + return; } - return null; - } -} - -function startsWith(commentText: string, changeCase: (str: string) => string) { - if (commentText.length <= 2) { - return true; // comment is "//"? Technically not a violation. - } - - // regex is "start of string"//"any amount of whitespace"("word character") - const firstCharacterMatch = commentText.match(/^\/\/\s*(\w)/); - if (firstCharacterMatch != null) { - // the first group matched, i.e. the thing in the parens, is the first non-space character, if it's alphanumeric - const firstCharacter = firstCharacterMatch[1]; - return firstCharacter === changeCase(firstCharacter); - } else { - // first character isn't alphanumeric/doesn't exist? Technically not a violation - return true; - } -} - -function startsWithLowercase(commentText: string) { - return startsWith(commentText, (c) => c.toLowerCase()); -} - -function startsWithUppercase(commentText: string) { - return startsWith(commentText, (c) => c.toUpperCase()); -} - -function startsWithSpace(commentText: string) { - if (commentText.length <= 2) { - return true; // comment is "//"? Technically not a violation. - } - - const commentBody = commentText.substring(2); - - // whitelist //#region and //#endregion - if ((/^#(end)?region/).test(commentBody)) { - return true; - } - // whitelist JetBrains IDEs' "//noinspection ..." - if ((/^noinspection\s/).test(commentBody)) { - return true; - } + if (ctx.options.space && commentText[0] !== " ") { + ctx.addFailure(start, end, Rule.LEADING_SPACE_FAILURE); + } - const firstCharacter = commentBody.charAt(0); - // three slashes (///) also works, to allow for /// - return firstCharacter === " " || firstCharacter === "/"; -} + if (ctx.options.case === Case.None || + ctx.options.exceptions !== undefined && ctx.options.exceptions.test(commentText) || + ENABLE_DISABLE_REGEX.test(commentText)) { + return; + } -function isEnableDisableFlag(commentText: string): boolean { - // regex is: start of string followed by "/*" or "//" - // followed by any amount of whitespace followed by "tslint:" - // followed by either "enable" or "disable" - return /^(\/\*|\/\/)\s*tslint:(enable|disable)/.test(commentText); + // search for first non-space character to check if lower or upper + const charPos = commentText.search(/\S/); + if (charPos === -1) { + return; + } + if (ctx.options.case === Case.Lower) { + if (!isLowerCase(commentText[charPos])) { + ctx.addFailure(start, end, Rule.LOWERCASE_FAILURE + ctx.options.failureSuffix); + } + } else if (!isUpperCase(commentText[charPos])) { + ctx.addFailure(start, end, Rule.UPPERCASE_FAILURE + ctx.options.failureSuffix); + } + }); } diff --git a/test/rules/comment-format/exceptions-pattern/test.ts.lint b/test/rules/comment-format/exceptions-pattern/test.ts.lint index 42adb252e09..51540de85f2 100644 --- a/test/rules/comment-format/exceptions-pattern/test.ts.lint +++ b/test/rules/comment-format/exceptions-pattern/test.ts.lint @@ -1,3 +1,4 @@ +/// class Clazz { // this comment is correct /* block comment * adada @@ -10,7 +11,9 @@ class Clazz { // this comment is correct console.log("test"); //this comment has no space ~~~~~~~~~~~~~~~~~~~~~~~~~ [space] } - /// + /// + ////foo + ~~~ [space] } //#region test diff --git a/test/rules/comment-format/exceptions-words/test.ts.lint b/test/rules/comment-format/exceptions-words/test.ts.lint index aa7103ff2a4..3ce531fdf82 100644 --- a/test/rules/comment-format/exceptions-words/test.ts.lint +++ b/test/rules/comment-format/exceptions-words/test.ts.lint @@ -10,7 +10,9 @@ class Clazz { // this comment is correct console.log("test"); //this comment has no space ~~~~~~~~~~~~~~~~~~~~~~~~~ [space] } - /// + /// + ////foo + ~~~ [space] } //#region test diff --git a/test/rules/comment-format/lower/test.ts.lint b/test/rules/comment-format/lower/test.ts.lint index 6b0f7e4cfb2..5b3467f2ba6 100644 --- a/test/rules/comment-format/lower/test.ts.lint +++ b/test/rules/comment-format/lower/test.ts.lint @@ -10,7 +10,9 @@ class Clazz { // this comment is correct console.log("test"); //this comment has no space ~~~~~~~~~~~~~~~~~~~~~~~~~ [space] } - /// + /// + ////foo + ~~~ [space] } //#region test diff --git a/test/rules/comment-format/upper/test.ts.lint b/test/rules/comment-format/upper/test.ts.lint index c94e6226882..94a205758af 100644 --- a/test/rules/comment-format/upper/test.ts.lint +++ b/test/rules/comment-format/upper/test.ts.lint @@ -10,7 +10,10 @@ class Clazz { // This comment is correct console.log("test"); //This comment has no space ~~~~~~~~~~~~~~~~~~~~~~~~~ [space] } - /// + /// + ////foo + ~~~ [space] + ~~~ [upper] } //#region test