From 53a9cbcd90c6a7703cbff7c2fd454eb432d6e4c1 Mon Sep 17 00:00:00 2001 From: smoehrle <26185187+smoehrle@users.noreply.github.com> Date: Fri, 15 Dec 2017 15:15:20 +0100 Subject: [PATCH 1/5] Change commentFormatRule to only emit one message per error --- src/rules/commentFormatRule.ts | 46 +++++++++++++++---- .../exceptions-pattern/test.ts.lint | 4 +- .../exceptions-words/test.ts.lint | 4 +- test/rules/comment-format/lower/test.ts.lint | 4 +- test/rules/comment-format/upper/test.ts.lint | 7 ++- 5 files changed, 46 insertions(+), 19 deletions(-) diff --git a/src/rules/commentFormatRule.ts b/src/rules/commentFormatRule.ts index d271dd779c3..ccbb7b6695e 100644 --- a/src/rules/commentFormatRule.ts +++ b/src/rules/commentFormatRule.ts @@ -34,6 +34,12 @@ interface Options { failureSuffix: string; } +interface Failures { + leadingSpace: boolean; + uppercase: boolean; + lowercase: boolean; +} + const enum Case { None, Lower, @@ -107,7 +113,9 @@ export class Rule extends Lint.Rules.AbstractRule { /* tslint:enable:object-literal-sort-keys */ public static LOWERCASE_FAILURE = "comment must start with lowercase letter"; + public static SPACE_LOWERCASE_FAILURE = "comment must start with a space and lowercase letter"; public static UPPERCASE_FAILURE = "comment must start with uppercase letter"; + public static SPACE_UPPERCASE_FAILURE = "comment must start with a space and uppercase letter"; public static LEADING_SPACE_FAILURE = "comment must start with a space"; public static IGNORE_WORDS_FAILURE_FACTORY = (words: string[]): string => ` or the word(s): ${words.join(", ")}`; @@ -185,10 +193,9 @@ function walk(ctx: Lint.WalkContext) { return; } + let failures: Failures = { leadingSpace: false, uppercase: false, lowercase: false}; if (ctx.options.space && commentText[0] !== " ") { - ctx.addFailure(start, end, Rule.LEADING_SPACE_FAILURE, [ - Lint.Replacement.appendText(start, " "), - ]); + failures.leadingSpace = true; } if ( @@ -196,20 +203,41 @@ function walk(ctx: Lint.WalkContext) { (ctx.options.exceptions !== undefined && ctx.options.exceptions.test(commentText)) || ENABLE_DISABLE_REGEX.test(commentText) ) { + addFailure(ctx, start, end, failures); return; } // search for first non-space character to check if lower or upper const charPos = commentText.search(/\S/); if (charPos === -1) { + addFailure(ctx, start, end, failures); 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); + if (ctx.options.case === Case.Lower && !isLowerCase(commentText[charPos])) { + failures.lowercase = true; + // ctx.addFailure(start, end, Rule.LOWERCASE_FAILURE + ctx.options.failureSuffix); + } else if (ctx.options.case === Case.Upper && !isUpperCase(commentText[charPos])) { + failures.uppercase = true; + // ctx.addFailure(start, end, Rule.UPPERCASE_FAILURE + ctx.options.failureSuffix); } + addFailure(ctx, start, end, failures); }); } + +function addFailure(ctx: Lint.WalkContext, start: number, end: number, failures: Failures) { + // No failure detected + if (!failures.leadingSpace && !failures.lowercase && !failures.uppercase) { + return; + } + + if (failures.lowercase) { + let msg = failures.leadingSpace ? Rule.SPACE_LOWERCASE_FAILURE : Rule.LOWERCASE_FAILURE; + ctx.addFailure(start, end, msg + ctx.options.failureSuffix); + } else if (failures.uppercase) { + let msg = failures.leadingSpace ? Rule.SPACE_UPPERCASE_FAILURE : Rule.UPPERCASE_FAILURE; + ctx.addFailure(start, end, msg + ctx.options.failureSuffix); + } else { + // Only whitespace failure + ctx.addFailure(start, end, Rule.LEADING_SPACE_FAILURE); + } +} \ No newline at end of file diff --git a/test/rules/comment-format/exceptions-pattern/test.ts.lint b/test/rules/comment-format/exceptions-pattern/test.ts.lint index 51540de85f2..58cb88eeabd 100644 --- a/test/rules/comment-format/exceptions-pattern/test.ts.lint +++ b/test/rules/comment-format/exceptions-pattern/test.ts.lint @@ -6,8 +6,7 @@ class Clazz { // this comment is correct public funcxion() { // This comment has a capital letter starting it ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [lower] //This comment is on its own line, and starts with a capital _and_ no space - ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [lower] - ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [space] + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [space_lower] console.log("test"); //this comment has no space ~~~~~~~~~~~~~~~~~~~~~~~~~ [space] } @@ -37,3 +36,4 @@ const unusedVar = 'unneeded value'; [lower]: comment must start with lowercase letter or its start must match the regex pattern "STD\w{2,3}" [space]: comment must start with a space +[space_lower]: comment must start with a space and lowercase letter or its start must match the regex pattern "STD\w{2,3}" diff --git a/test/rules/comment-format/exceptions-words/test.ts.lint b/test/rules/comment-format/exceptions-words/test.ts.lint index 3ce531fdf82..4a19e036682 100644 --- a/test/rules/comment-format/exceptions-words/test.ts.lint +++ b/test/rules/comment-format/exceptions-words/test.ts.lint @@ -5,8 +5,7 @@ class Clazz { // this comment is correct public funcxion() { // This comment has a capital letter starting it ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [lower] //This comment is on its own line, and starts with a capital _and_ no space - ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [lower] - ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [space] + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [space_lower] console.log("test"); //this comment has no space ~~~~~~~~~~~~~~~~~~~~~~~~~ [space] } @@ -37,3 +36,4 @@ const unusedVar = 'unneeded value'; [lower]: comment must start with lowercase letter or the word(s): TODO, HACK [space]: comment must start with a space +[space_lower]: comment must start with a space and lowercase letter or the word(s): TODO, HACK diff --git a/test/rules/comment-format/lower/test.ts.lint b/test/rules/comment-format/lower/test.ts.lint index 5b3467f2ba6..07c3390abf6 100644 --- a/test/rules/comment-format/lower/test.ts.lint +++ b/test/rules/comment-format/lower/test.ts.lint @@ -5,8 +5,7 @@ class Clazz { // this comment is correct public funcxion() { // This comment has a capital letter starting it ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [lower] //This comment is on its own line, and starts with a capital _and_ no space - ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [lower] - ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [space] + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [space_lower] console.log("test"); //this comment has no space ~~~~~~~~~~~~~~~~~~~~~~~~~ [space] } @@ -25,3 +24,4 @@ const unusedVar = 'unneeded value'; [lower]: comment must start with lowercase letter [space]: comment must start with a space +[space_lower]: comment must start with a space and lowercase letter diff --git a/test/rules/comment-format/upper/test.ts.lint b/test/rules/comment-format/upper/test.ts.lint index 94a205758af..d9d21816446 100644 --- a/test/rules/comment-format/upper/test.ts.lint +++ b/test/rules/comment-format/upper/test.ts.lint @@ -5,15 +5,13 @@ class Clazz { // This comment is correct public funcxion() { // this comment has a lowercase letter starting it ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [upper] //this comment is on its own line, and starts with a lowercase _and_ no space - ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [upper] - ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [space] + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [space_upper] console.log("test"); //This comment has no space ~~~~~~~~~~~~~~~~~~~~~~~~~ [space] } /// ////foo - ~~~ [space] - ~~~ [upper] + ~~~ [space_upper] } //#region test @@ -36,3 +34,4 @@ class Valid {} [upper]: comment must start with uppercase letter [space]: comment must start with a space +[space_upper]: comment must start with a space and uppercase letter From d5055b46b5dfb49a5ea70abdc79c115bc0477bfe Mon Sep 17 00:00:00 2001 From: smoehrle <26185187+smoehrle@users.noreply.github.com> Date: Fri, 15 Dec 2017 15:15:29 +0100 Subject: [PATCH 2/5] Add fixer --- src/rules/commentFormatRule.ts | 46 +++++++++++++------ .../exceptions-pattern/test.ts.fix | 8 ++-- .../exceptions-words/test.ts.fix | 29 ++++++++++++ test/rules/comment-format/lower/test.ts.fix | 20 ++++++++ test/rules/comment-format/lower/test.tsx.fix | 9 ++++ test/rules/comment-format/upper/test.ts.fix | 27 +++++++++++ 6 files changed, 122 insertions(+), 17 deletions(-) create mode 100644 test/rules/comment-format/exceptions-words/test.ts.fix create mode 100644 test/rules/comment-format/lower/test.ts.fix create mode 100644 test/rules/comment-format/lower/test.tsx.fix create mode 100644 test/rules/comment-format/upper/test.ts.fix diff --git a/src/rules/commentFormatRule.ts b/src/rules/commentFormatRule.ts index ccbb7b6695e..8501430d087 100644 --- a/src/rules/commentFormatRule.ts +++ b/src/rules/commentFormatRule.ts @@ -38,6 +38,7 @@ interface Failures { leadingSpace: boolean; uppercase: boolean; lowercase: boolean; + firstLetterPos: number; } const enum Case { @@ -193,7 +194,7 @@ function walk(ctx: Lint.WalkContext) { return; } - let failures: Failures = { leadingSpace: false, uppercase: false, lowercase: false}; + const failures: Failures = { leadingSpace: false, uppercase: false, lowercase: false, firstLetterPos: -1}; if (ctx.options.space && commentText[0] !== " ") { failures.leadingSpace = true; } @@ -203,41 +204,60 @@ function walk(ctx: Lint.WalkContext) { (ctx.options.exceptions !== undefined && ctx.options.exceptions.test(commentText)) || ENABLE_DISABLE_REGEX.test(commentText) ) { - addFailure(ctx, start, end, failures); + addFailure(ctx, commentText, start, end, failures); return; } // search for first non-space character to check if lower or upper const charPos = commentText.search(/\S/); if (charPos === -1) { - addFailure(ctx, start, end, failures); + addFailure(ctx, commentText, start, end, failures); return; } + failures.firstLetterPos = charPos; if (ctx.options.case === Case.Lower && !isLowerCase(commentText[charPos])) { failures.lowercase = true; - // ctx.addFailure(start, end, Rule.LOWERCASE_FAILURE + ctx.options.failureSuffix); } else if (ctx.options.case === Case.Upper && !isUpperCase(commentText[charPos])) { failures.uppercase = true; - // ctx.addFailure(start, end, Rule.UPPERCASE_FAILURE + ctx.options.failureSuffix); } - addFailure(ctx, start, end, failures); + addFailure(ctx, commentText, start, end, failures); }); } -function addFailure(ctx: Lint.WalkContext, start: number, end: number, failures: Failures) { +function addFailure(ctx: Lint.WalkContext, comment: string, start: number, end: number, failures: Failures) { // No failure detected if (!failures.leadingSpace && !failures.lowercase && !failures.uppercase) { return; } if (failures.lowercase) { - let msg = failures.leadingSpace ? Rule.SPACE_LOWERCASE_FAILURE : Rule.LOWERCASE_FAILURE; - ctx.addFailure(start, end, msg + ctx.options.failureSuffix); + const msg = failures.leadingSpace ? Rule.SPACE_LOWERCASE_FAILURE : Rule.LOWERCASE_FAILURE; + const fix = generateFix(comment, start, failures); + ctx.addFailure(start, end, msg + ctx.options.failureSuffix, fix); } else if (failures.uppercase) { - let msg = failures.leadingSpace ? Rule.SPACE_UPPERCASE_FAILURE : Rule.UPPERCASE_FAILURE; - ctx.addFailure(start, end, msg + ctx.options.failureSuffix); + const msg = failures.leadingSpace ? Rule.SPACE_UPPERCASE_FAILURE : Rule.UPPERCASE_FAILURE; + const fix = generateFix(comment, start, failures); + ctx.addFailure(start, end, msg + ctx.options.failureSuffix, fix); } else { // Only whitespace failure - ctx.addFailure(start, end, Rule.LEADING_SPACE_FAILURE); + ctx.addFailure(start, end, Rule.LEADING_SPACE_FAILURE, generateFix(comment, start, failures)); } -} \ No newline at end of file +} + +function generateFix(comment: string, start: number, failures: Failures) { + if (failures.lowercase) { + const fix = comment[failures.firstLetterPos].toLowerCase(); + if (failures.leadingSpace) { + return new Lint.Replacement(start, 1, ` ${fix}`); + } + return new Lint.Replacement(start + failures.firstLetterPos, 1, fix); + } else if (failures.uppercase) { + const fix = comment[failures.firstLetterPos].toUpperCase(); + if (failures.leadingSpace) { + return new Lint.Replacement(start, 1, ` ${fix}`); + } + return new Lint.Replacement(start + failures.firstLetterPos, 1, fix); + } else { + return Lint.Replacement.appendText(start, " "); + } +} diff --git a/test/rules/comment-format/exceptions-pattern/test.ts.fix b/test/rules/comment-format/exceptions-pattern/test.ts.fix index 546088ec95b..c142d9cb849 100644 --- a/test/rules/comment-format/exceptions-pattern/test.ts.fix +++ b/test/rules/comment-format/exceptions-pattern/test.ts.fix @@ -3,8 +3,8 @@ class Clazz { // this comment is correct /* block comment * adada */ - public funcxion() { // This comment has a capital letter starting it - // This comment is on its own line, and starts with a capital _and_ no space + public funcxion() { // this comment has a capital letter starting it + // this comment is on its own line, and starts with a capital _and_ no space console.log("test"); // this comment has no space } /// @@ -19,8 +19,8 @@ class Clazz { // this comment is correct //noinspection JSUnusedGlobalSymbols const unusedVar = 'unneeded value'; -// TODO: Write more tests -// HACKING is not an exception +// tODO: Write more tests +// hACKING is not an exception // STDIN for input // STDOUT for output diff --git a/test/rules/comment-format/exceptions-words/test.ts.fix b/test/rules/comment-format/exceptions-words/test.ts.fix new file mode 100644 index 00000000000..ff593a9c2fe --- /dev/null +++ b/test/rules/comment-format/exceptions-words/test.ts.fix @@ -0,0 +1,29 @@ +class Clazz { // this comment is correct + /* block comment + * adada + */ + public funcxion() { // this comment has a capital letter starting it + // this comment is on its own line, and starts with a capital _and_ no space + console.log("test"); // this comment has no space + } + /// + //// foo +} + +//#region test +//#endregion + +`${location.protocol}//${location.hostname}` + +//noinspection JSUnusedGlobalSymbols +const unusedVar = 'unneeded value'; + +// TODO: Write more tests + +// hACKING is not an exception + +// sTDIN for input +// sTDOUT for output +// stderr for errors + + diff --git a/test/rules/comment-format/lower/test.ts.fix b/test/rules/comment-format/lower/test.ts.fix new file mode 100644 index 00000000000..d71c05768db --- /dev/null +++ b/test/rules/comment-format/lower/test.ts.fix @@ -0,0 +1,20 @@ +class Clazz { // this comment is correct + /* block comment + * adada + */ + public funcxion() { // this comment has a capital letter starting it + // this comment is on its own line, and starts with a capital _and_ no space + console.log("test"); // this comment has no space + } + /// + //// foo +} + +//#region test +//#endregion + +`${location.protocol}//${location.hostname}` + +//noinspection JSUnusedGlobalSymbols +const unusedVar = 'unneeded value'; + diff --git a/test/rules/comment-format/lower/test.tsx.fix b/test/rules/comment-format/lower/test.tsx.fix new file mode 100644 index 00000000000..7c09e671f03 --- /dev/null +++ b/test/rules/comment-format/lower/test.tsx.fix @@ -0,0 +1,9 @@ +const a = ( + + https://github.com/ { + // invalid comment + content + }, text + +); + diff --git a/test/rules/comment-format/upper/test.ts.fix b/test/rules/comment-format/upper/test.ts.fix new file mode 100644 index 00000000000..562af0396fa --- /dev/null +++ b/test/rules/comment-format/upper/test.ts.fix @@ -0,0 +1,27 @@ +class Clazz { // This comment is correct + /* block comment + * adada + */ + public funcxion() { // This comment has a lowercase letter starting it + // This comment is on its own line, and starts with a lowercase _and_ no space + console.log("test"); // This comment has no space + } + /// + //// Foo +} + +//#region test +//#endregion + +`${location.protocol}//${location.hostname}` + +// Tslint should show error here + +// Tslint: not a rule flag + +// tslint:disable-next-line:no-unused-expression +class Invalid {} + +// tslint:disable-next-line:no-unused-expression +class Valid {} + From 9f2a97b3d0fb6b071542885a8a3ff789fae0a789 Mon Sep 17 00:00:00 2001 From: smoehrle <26185187+smoehrle@users.noreply.github.com> Date: Fri, 15 Dec 2017 15:15:33 +0100 Subject: [PATCH 3/5] Add allow-trailing-lowercase option --- src/rules/commentFormatRule.ts | 57 ++++++++++++++----- .../allow-trailing-lowercase/test.ts.fix | 9 +++ .../allow-trailing-lowercase/test.ts.lint | 13 +++++ .../allow-trailing-lowercase/tslint.json | 5 ++ 4 files changed, 69 insertions(+), 15 deletions(-) create mode 100644 test/rules/comment-format/allow-trailing-lowercase/test.ts.fix create mode 100644 test/rules/comment-format/allow-trailing-lowercase/test.ts.lint create mode 100644 test/rules/comment-format/allow-trailing-lowercase/tslint.json diff --git a/src/rules/commentFormatRule.ts b/src/rules/commentFormatRule.ts index 8501430d087..81ffb4d9816 100644 --- a/src/rules/commentFormatRule.ts +++ b/src/rules/commentFormatRule.ts @@ -32,6 +32,7 @@ interface Options { case: Case; exceptions?: RegExp; failureSuffix: string; + allowTrailingLowercase: boolean; } interface Failures { @@ -50,6 +51,7 @@ const enum Case { const OPTION_SPACE = "check-space"; const OPTION_LOWERCASE = "check-lowercase"; const OPTION_UPPERCASE = "check-uppercase"; +const OPTION_ALLOW_TRAILING_LOWERCASE = "allow-trailing-lowercase"; export class Rule extends Lint.Rules.AbstractRule { /* tslint:disable:object-literal-sort-keys */ @@ -58,20 +60,24 @@ export class Rule extends Lint.Rules.AbstractRule { description: "Enforces formatting rules for single-line comments.", rationale: "Helps maintain a consistent, readable style in your codebase.", optionsDescription: Lint.Utils.dedent` - Three arguments may be optionally provided: + Four arguments may be optionally provided: - * \`"check-space"\` requires that all single-line comments must begin with a space, as in \`// comment\` + * \`"${OPTION_SPACE}"\` requires that all single-line comments must begin with a space, as in \`// comment\` * 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. + * \`"${OPTION_LOWERCASE}"\` requires that the first non-whitespace character of a comment must be lowercase, if applicable. + * \`"${OPTION_UPPERCASE}"\` requires that the first non-whitespace character of a comment must be uppercase, if applicable. + * \`"${OPTION_ALLOW_TRAILING_LOWERCASE}"\` allows that only the first comment of a series of comments needs to be uppercase. + * requires \`"${OPTION_UPPERCASE}"\` + * comments must start at the same position - Exceptions to \`"check-lowercase"\` or \`"check-uppercase"\` can be managed with object that may be passed as last argument. + Exceptions to \`"${OPTION_LOWERCASE}"\` or \`"${OPTION_UPPERCASE}"\` can be managed with object that may be passed as last + argument. One of two options can be provided in this object: - * \`"ignore-words"\` - array of strings - words that will be ignored at the beginning of the comment. - * \`"ignore-pattern"\` - string - RegExp pattern that will be ignored at the beginning of the comment. + * \`"ignore-words"\` - array of strings - words that will be ignored at the beginning of the comment. + * \`"ignore-pattern"\` - string - RegExp pattern that will be ignored at the beginning of the comment. `, options: { type: "array", @@ -79,7 +85,12 @@ export class Rule extends Lint.Rules.AbstractRule { anyOf: [ { type: "string", - enum: ["check-space", "check-lowercase", "check-uppercase"], + enum: [ + OPTION_SPACE, + OPTION_LOWERCASE, + OPTION_UPPERCASE, + OPTION_ALLOW_TRAILING_LOWERCASE, + ], }, { type: "object", @@ -100,12 +111,12 @@ export class Rule extends Lint.Rules.AbstractRule { ], }, minLength: 1, - maxLength: 4, + maxLength: 5, }, optionExamples: [ - [true, "check-space", "check-uppercase"], - [true, "check-lowercase", { "ignore-words": ["TODO", "HACK"] }], - [true, "check-lowercase", { "ignore-pattern": "STD\\w{2,3}\\b" }], + [true, OPTION_SPACE, OPTION_UPPERCASE, OPTION_ALLOW_TRAILING_LOWERCASE], + [true, OPTION_LOWERCASE, { "ignore-words": ["TODO", "HACK"] }], + [true, OPTION_LOWERCASE, { "ignore-pattern": "STD\\w{2,3}\\b" }], ], type: "style", typescriptOnly: false, @@ -130,16 +141,21 @@ export class Rule extends Lint.Rules.AbstractRule { function parseOptions(options: Array): Options { return { + allowTrailingLowercase: has(OPTION_ALLOW_TRAILING_LOWERCASE), case: - options.indexOf(OPTION_LOWERCASE) !== -1 + has(OPTION_LOWERCASE) ? Case.Lower - : options.indexOf(OPTION_UPPERCASE) !== -1 + : has(OPTION_UPPERCASE) ? Case.Upper : Case.None, failureSuffix: "", - space: options.indexOf(OPTION_SPACE) !== -1, + space: has(OPTION_SPACE), ...composeExceptions(options[options.length - 1]), }; + + function has(option: string): boolean { + return options.indexOf(option) !== -1; + } } function composeExceptions( @@ -181,6 +197,7 @@ function walk(ctx: Lint.WalkContext) { ) { return; } + // skip all leading slashes while (fullText[start] === "/") { ++start; @@ -219,6 +236,16 @@ function walk(ctx: Lint.WalkContext) { failures.lowercase = true; } else if (ctx.options.case === Case.Upper && !isUpperCase(commentText[charPos])) { failures.uppercase = true; + if (ctx.options.allowTrailingLowercase) { + const lineAndCharCurrentComment = ts.getLineAndCharacterOfPosition(ctx.sourceFile, pos); + const posPrevComment = ctx.sourceFile.getPositionOfLineAndCharacter( + lineAndCharCurrentComment.line - 1, 0) + lineAndCharCurrentComment.character; + + const prevComment = utils.getCommentAtPosition(ctx.sourceFile, posPrevComment); + if (prevComment !== undefined && prevComment.pos === posPrevComment) { + failures.uppercase = false; + } + } } addFailure(ctx, commentText, start, end, failures); }); diff --git a/test/rules/comment-format/allow-trailing-lowercase/test.ts.fix b/test/rules/comment-format/allow-trailing-lowercase/test.ts.fix new file mode 100644 index 00000000000..57a3fa3f83d --- /dev/null +++ b/test/rules/comment-format/allow-trailing-lowercase/test.ts.fix @@ -0,0 +1,9 @@ +// This is a valid +// multiline comment + +// This is an invalid +// multiline + +let a = 1; // Some description +let b = "foo"; // Another description + diff --git a/test/rules/comment-format/allow-trailing-lowercase/test.ts.lint b/test/rules/comment-format/allow-trailing-lowercase/test.ts.lint new file mode 100644 index 00000000000..238aa4cdf7d --- /dev/null +++ b/test/rules/comment-format/allow-trailing-lowercase/test.ts.lint @@ -0,0 +1,13 @@ +// This is a valid +// multiline comment + +// This is an invalid +//multiline + ~~~~~~~~~[space] + +let a = 1; // Some description +let b = "foo"; // another description + ~~~~~~~~~~~~~~~~~~~~[upper] + +[upper]: comment must start with uppercase letter +[space]: comment must start with a space diff --git a/test/rules/comment-format/allow-trailing-lowercase/tslint.json b/test/rules/comment-format/allow-trailing-lowercase/tslint.json new file mode 100644 index 00000000000..705d0c25fc8 --- /dev/null +++ b/test/rules/comment-format/allow-trailing-lowercase/tslint.json @@ -0,0 +1,5 @@ +{ + "rules": { + "comment-format": [true, "check-space", "check-uppercase", "allow-trailing-lowercase"] + } +} From 3ca099ba3ce32e6674592fa11b0aa46e1fde9e49 Mon Sep 17 00:00:00 2001 From: smoehrle <26185187+smoehrle@users.noreply.github.com> Date: Wed, 20 Dec 2017 13:55:41 +0100 Subject: [PATCH 4/5] Changes after review: cache last comment, rework on structure --- src/rules/commentFormatRule.ts | 163 +++++++++++++++++---------------- 1 file changed, 86 insertions(+), 77 deletions(-) diff --git a/src/rules/commentFormatRule.ts b/src/rules/commentFormatRule.ts index 81ffb4d9816..89ab2c5e04e 100644 --- a/src/rules/commentFormatRule.ts +++ b/src/rules/commentFormatRule.ts @@ -21,6 +21,7 @@ import * as ts from "typescript"; import { ENABLE_DISABLE_REGEX } from "../enableDisableRules"; import * as Lint from "../index"; import { escapeRegExp, isLowerCase, isUpperCase } from "../utils"; +import { LineAndCharacter } from 'typescript'; interface IExceptionsObject { "ignore-words"?: string[]; @@ -35,10 +36,12 @@ interface Options { allowTrailingLowercase: boolean; } -interface Failures { - leadingSpace: boolean; - uppercase: boolean; - lowercase: boolean; +interface CommentStatus { + text: string; + start: number; + leadingSpaceError: boolean; + uppercaseError: boolean; + lowercaseError: boolean; firstLetterPos: number; } @@ -135,7 +138,8 @@ export class Rule extends Lint.Rules.AbstractRule { ` or its start must match the regex pattern "${pattern}"`; public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { - return this.applyWithFunction(sourceFile, walk, parseOptions(this.ruleArguments)); + const commentFormatWalker = new CommentFormatWalker(sourceFile, this.ruleName, parseOptions(this.ruleArguments)); + return this.applyWithWalker(commentFormatWalker); } } @@ -184,107 +188,112 @@ function composeExceptions( return undefined; } -function walk(ctx: Lint.WalkContext) { - utils.forEachComment(ctx.sourceFile, (fullText, { kind, pos, end }) => { - let start = pos + 2; +class CommentFormatWalker extends Lint.AbstractWalker { + private prevComment: LineAndCharacter; + + public walk(sourceFile: ts.SourceFile) { + utils.forEachComment(sourceFile, (fullText, comment) => { + const commentStatus = this.checkComment(fullText, comment); + this.handleFailure(commentStatus, comment.end); + // cache position of last comment + this.prevComment = ts.getLineAndCharacterOfPosition(sourceFile, comment.pos); + }); + } + + private checkComment(fullText: string, {kind, pos, end}: ts.CommentRange): CommentStatus { + const status: CommentStatus = { + text: "", + start: pos + 2, + leadingSpaceError: false, + uppercaseError: false, + lowercaseError: false, + firstLetterPos: -1 + }; + if ( kind !== ts.SyntaxKind.SingleLineCommentTrivia || // exclude empty comments - start === end || + status.start === end || // exclude /// - (fullText[start] === "/" && - ctx.sourceFile.referencedFiles.some(ref => ref.pos >= pos && ref.end <= end)) + (fullText[status.start] === "/" && + this.sourceFile.referencedFiles.some((ref) => ref.pos >= pos && ref.end <= end)) ) { - return; + return status; } // skip all leading slashes - while (fullText[start] === "/") { - ++start; + while (fullText[status.start] === "/") { + ++status.start; } - if (start === end) { - return; + if (status.start === end) { + return status; } - const commentText = fullText.slice(start, end); + status.text = fullText.slice(status.start, end); // whitelist //#region and //#endregion and JetBrains IDEs' "//noinspection ..." - if (/^(?:#(?:end)?region|noinspection\s)/.test(commentText)) { - return; + if (/^(?:#(?:end)?region|noinspection\s)/.test(status.text)) { + return status; } - const failures: Failures = { leadingSpace: false, uppercase: false, lowercase: false, firstLetterPos: -1}; - if (ctx.options.space && commentText[0] !== " ") { - failures.leadingSpace = true; + if (this.options.space && status.text[0] !== " ") { + status.leadingSpaceError = true; } if ( - ctx.options.case === Case.None || - (ctx.options.exceptions !== undefined && ctx.options.exceptions.test(commentText)) || - ENABLE_DISABLE_REGEX.test(commentText) + this.options.case === Case.None || + (this.options.exceptions !== undefined && this.options.exceptions.test(status.text)) || + ENABLE_DISABLE_REGEX.test(status.text) ) { - addFailure(ctx, commentText, start, end, failures); - return; + return status; } // search for first non-space character to check if lower or upper - const charPos = commentText.search(/\S/); + const charPos = status.text.search(/\S/); if (charPos === -1) { - addFailure(ctx, commentText, start, end, failures); - return; + return status; } - failures.firstLetterPos = charPos; - if (ctx.options.case === Case.Lower && !isLowerCase(commentText[charPos])) { - failures.lowercase = true; - } else if (ctx.options.case === Case.Upper && !isUpperCase(commentText[charPos])) { - failures.uppercase = true; - if (ctx.options.allowTrailingLowercase) { - const lineAndCharCurrentComment = ts.getLineAndCharacterOfPosition(ctx.sourceFile, pos); - const posPrevComment = ctx.sourceFile.getPositionOfLineAndCharacter( - lineAndCharCurrentComment.line - 1, 0) + lineAndCharCurrentComment.character; - - const prevComment = utils.getCommentAtPosition(ctx.sourceFile, posPrevComment); - if (prevComment !== undefined && prevComment.pos === posPrevComment) { - failures.uppercase = false; + status.firstLetterPos = charPos; + if (this.options.case === Case.Lower && !isLowerCase(status.text[charPos])) { + status.lowercaseError = true; + } else if (this.options.case === Case.Upper && !isUpperCase(status.text[charPos])) { + status.uppercaseError = true; + if (this.options.allowTrailingLowercase && this.prevComment !== undefined) { + const currentComment = ts.getLineAndCharacterOfPosition(this.sourceFile, pos); + if (this.prevComment.line + 1 === currentComment.line + && this.prevComment.character === currentComment.character) { + status.uppercaseError = false; } } } - addFailure(ctx, commentText, start, end, failures); - }); -} - -function addFailure(ctx: Lint.WalkContext, comment: string, start: number, end: number, failures: Failures) { - // No failure detected - if (!failures.leadingSpace && !failures.lowercase && !failures.uppercase) { - return; + return status; } - if (failures.lowercase) { - const msg = failures.leadingSpace ? Rule.SPACE_LOWERCASE_FAILURE : Rule.LOWERCASE_FAILURE; - const fix = generateFix(comment, start, failures); - ctx.addFailure(start, end, msg + ctx.options.failureSuffix, fix); - } else if (failures.uppercase) { - const msg = failures.leadingSpace ? Rule.SPACE_UPPERCASE_FAILURE : Rule.UPPERCASE_FAILURE; - const fix = generateFix(comment, start, failures); - ctx.addFailure(start, end, msg + ctx.options.failureSuffix, fix); - } else { - // Only whitespace failure - ctx.addFailure(start, end, Rule.LEADING_SPACE_FAILURE, generateFix(comment, start, failures)); - } -} -function generateFix(comment: string, start: number, failures: Failures) { - if (failures.lowercase) { - const fix = comment[failures.firstLetterPos].toLowerCase(); - if (failures.leadingSpace) { - return new Lint.Replacement(start, 1, ` ${fix}`); + private handleFailure(status: CommentStatus, end: number) { + // No failure detected + if (!status.leadingSpaceError && !status.lowercaseError && !status.uppercaseError) { + return; } - return new Lint.Replacement(start + failures.firstLetterPos, 1, fix); - } else if (failures.uppercase) { - const fix = comment[failures.firstLetterPos].toUpperCase(); - if (failures.leadingSpace) { - return new Lint.Replacement(start, 1, ` ${fix}`); + + // Only whitespace failure + if (status.leadingSpaceError && !status.lowercaseError && !status.uppercaseError) { + this.addFailure(status.start, end, Rule.LEADING_SPACE_FAILURE, Lint.Replacement.appendText(status.start, " ")); + return; } - return new Lint.Replacement(start + failures.firstLetterPos, 1, fix); - } else { - return Lint.Replacement.appendText(start, " "); + + let msg: string; + let firstLetterFix: string + + if (status.lowercaseError) { + msg = status.leadingSpaceError ? Rule.SPACE_LOWERCASE_FAILURE : Rule.LOWERCASE_FAILURE; + firstLetterFix = status.text[status.firstLetterPos].toLowerCase(); + } else { + msg = status.leadingSpaceError ? Rule.SPACE_UPPERCASE_FAILURE : Rule.UPPERCASE_FAILURE; + firstLetterFix = status.text[status.firstLetterPos].toUpperCase(); + } + + const fix = status.leadingSpaceError + ? new Lint.Replacement(status.start, 1, ` ${firstLetterFix}`) + : new Lint.Replacement(status.start + status.firstLetterPos, 1, firstLetterFix); + this.addFailure(status.start, end, msg + this.options.failureSuffix, fix); } } From a314ceef423e0e1d6bf09598b1d12e0db166ba66 Mon Sep 17 00:00:00 2001 From: smoehrle <26185187+smoehrle@users.noreply.github.com> Date: Wed, 20 Dec 2017 14:09:09 +0100 Subject: [PATCH 5/5] Make allow-trailing-lowercase a bit stricter --- src/rules/commentFormatRule.ts | 56 ++++++++++++------- .../allow-trailing-lowercase/test.ts.fix | 6 ++ .../allow-trailing-lowercase/test.ts.lint | 7 +++ 3 files changed, 48 insertions(+), 21 deletions(-) diff --git a/src/rules/commentFormatRule.ts b/src/rules/commentFormatRule.ts index 89ab2c5e04e..2c089d7232b 100644 --- a/src/rules/commentFormatRule.ts +++ b/src/rules/commentFormatRule.ts @@ -21,7 +21,6 @@ import * as ts from "typescript"; import { ENABLE_DISABLE_REGEX } from "../enableDisableRules"; import * as Lint from "../index"; import { escapeRegExp, isLowerCase, isUpperCase } from "../utils"; -import { LineAndCharacter } from 'typescript'; interface IExceptionsObject { "ignore-words"?: string[]; @@ -43,6 +42,7 @@ interface CommentStatus { uppercaseError: boolean; lowercaseError: boolean; firstLetterPos: number; + validForTrailingLowercase: boolean; } const enum Case { @@ -138,7 +138,11 @@ export class Rule extends Lint.Rules.AbstractRule { ` or its start must match the regex pattern "${pattern}"`; public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { - const commentFormatWalker = new CommentFormatWalker(sourceFile, this.ruleName, parseOptions(this.ruleArguments)); + const commentFormatWalker = new CommentFormatWalker( + sourceFile, + this.ruleName, + parseOptions(this.ruleArguments), + ); return this.applyWithWalker(commentFormatWalker); } } @@ -146,12 +150,7 @@ export class Rule extends Lint.Rules.AbstractRule { function parseOptions(options: Array): Options { return { allowTrailingLowercase: has(OPTION_ALLOW_TRAILING_LOWERCASE), - case: - has(OPTION_LOWERCASE) - ? Case.Lower - : has(OPTION_UPPERCASE) - ? Case.Upper - : Case.None, + case: has(OPTION_LOWERCASE) ? Case.Lower : has(OPTION_UPPERCASE) ? Case.Upper : Case.None, failureSuffix: "", space: has(OPTION_SPACE), ...composeExceptions(options[options.length - 1]), @@ -189,7 +188,8 @@ function composeExceptions( } class CommentFormatWalker extends Lint.AbstractWalker { - private prevComment: LineAndCharacter; + private prevComment: ts.LineAndCharacter | undefined; + private prevCommentIsValid: boolean | undefined; public walk(sourceFile: ts.SourceFile) { utils.forEachComment(sourceFile, (fullText, comment) => { @@ -197,17 +197,19 @@ class CommentFormatWalker extends Lint.AbstractWalker { this.handleFailure(commentStatus, comment.end); // cache position of last comment this.prevComment = ts.getLineAndCharacterOfPosition(sourceFile, comment.pos); + this.prevCommentIsValid = commentStatus.validForTrailingLowercase; }); } - private checkComment(fullText: string, {kind, pos, end}: ts.CommentRange): CommentStatus { + private checkComment(fullText: string, { kind, pos, end }: ts.CommentRange): CommentStatus { const status: CommentStatus = { - text: "", - start: pos + 2, + firstLetterPos: -1, leadingSpaceError: false, - uppercaseError: false, lowercaseError: false, - firstLetterPos: -1 + start: pos + 2, + text: "", + uppercaseError: false, + validForTrailingLowercase: false, }; if ( @@ -216,7 +218,7 @@ class CommentFormatWalker extends Lint.AbstractWalker { status.start === end || // exclude /// (fullText[status.start] === "/" && - this.sourceFile.referencedFiles.some((ref) => ref.pos >= pos && ref.end <= end)) + this.sourceFile.referencedFiles.some(ref => ref.pos >= pos && ref.end <= end)) ) { return status; } @@ -251,15 +253,23 @@ class CommentFormatWalker extends Lint.AbstractWalker { if (charPos === -1) { return status; } + // All non-empty and not whitelisted comments are valid for the trailing lowercase rule + status.validForTrailingLowercase = true; status.firstLetterPos = charPos; if (this.options.case === Case.Lower && !isLowerCase(status.text[charPos])) { status.lowercaseError = true; } else if (this.options.case === Case.Upper && !isUpperCase(status.text[charPos])) { status.uppercaseError = true; - if (this.options.allowTrailingLowercase && this.prevComment !== undefined) { + if ( + this.options.allowTrailingLowercase && + this.prevComment !== undefined && + this.prevCommentIsValid + ) { const currentComment = ts.getLineAndCharacterOfPosition(this.sourceFile, pos); - if (this.prevComment.line + 1 === currentComment.line - && this.prevComment.character === currentComment.character) { + if ( + this.prevComment.line + 1 === currentComment.line && + this.prevComment.character === currentComment.character + ) { status.uppercaseError = false; } } @@ -267,7 +277,6 @@ class CommentFormatWalker extends Lint.AbstractWalker { return status; } - private handleFailure(status: CommentStatus, end: number) { // No failure detected if (!status.leadingSpaceError && !status.lowercaseError && !status.uppercaseError) { @@ -276,12 +285,17 @@ class CommentFormatWalker extends Lint.AbstractWalker { // Only whitespace failure if (status.leadingSpaceError && !status.lowercaseError && !status.uppercaseError) { - this.addFailure(status.start, end, Rule.LEADING_SPACE_FAILURE, Lint.Replacement.appendText(status.start, " ")); + this.addFailure( + status.start, + end, + Rule.LEADING_SPACE_FAILURE, + Lint.Replacement.appendText(status.start, " "), + ); return; } let msg: string; - let firstLetterFix: string + let firstLetterFix: string; if (status.lowercaseError) { msg = status.leadingSpaceError ? Rule.SPACE_LOWERCASE_FAILURE : Rule.LOWERCASE_FAILURE; diff --git a/test/rules/comment-format/allow-trailing-lowercase/test.ts.fix b/test/rules/comment-format/allow-trailing-lowercase/test.ts.fix index 57a3fa3f83d..29c8535bbd0 100644 --- a/test/rules/comment-format/allow-trailing-lowercase/test.ts.fix +++ b/test/rules/comment-format/allow-trailing-lowercase/test.ts.fix @@ -7,3 +7,9 @@ let a = 1; // Some description let b = "foo"; // Another description +// Another multiline +// comment .. +// +// With multiple +// paragraphs + diff --git a/test/rules/comment-format/allow-trailing-lowercase/test.ts.lint b/test/rules/comment-format/allow-trailing-lowercase/test.ts.lint index 238aa4cdf7d..f53db41854f 100644 --- a/test/rules/comment-format/allow-trailing-lowercase/test.ts.lint +++ b/test/rules/comment-format/allow-trailing-lowercase/test.ts.lint @@ -9,5 +9,12 @@ let a = 1; // Some description let b = "foo"; // another description ~~~~~~~~~~~~~~~~~~~~[upper] +// Another multiline +// comment .. +// +// with multiple + ~~~~~~~~~~~~~~[upper] +// paragraphs + [upper]: comment must start with uppercase letter [space]: comment must start with a space