From 8a0fd1899f544470a35afb3117f4c71aad7e4e42 Mon Sep 17 00:00:00 2001 From: Dimitri Mitropoulos Date: Sat, 30 May 2020 17:54:02 -0400 Subject: [PATCH] feat(eslint-plugin): [ban-ts-comments] add "allow-with-description" option (#2099) --- packages/eslint-plugin/README.md | 2 +- .../docs/rules/ban-ts-comment.md | 58 +++++++- .../eslint-plugin/src/rules/ban-ts-comment.ts | 90 +++++++++--- .../tests/rules/ban-ts-comment.test.ts | 129 +++++++++++++++++- 4 files changed, 255 insertions(+), 24 deletions(-) diff --git a/packages/eslint-plugin/README.md b/packages/eslint-plugin/README.md index b32cb4a1782..975bb88f08b 100644 --- a/packages/eslint-plugin/README.md +++ b/packages/eslint-plugin/README.md @@ -92,7 +92,7 @@ Pro Tip: For larger codebases you may want to consider splitting our linting int | [`@typescript-eslint/adjacent-overload-signatures`](./docs/rules/adjacent-overload-signatures.md) | Require that member overloads be consecutive | :heavy_check_mark: | | | | [`@typescript-eslint/array-type`](./docs/rules/array-type.md) | Requires using either `T[]` or `Array` for arrays | | :wrench: | | | [`@typescript-eslint/await-thenable`](./docs/rules/await-thenable.md) | Disallows awaiting a value that is not a Thenable | :heavy_check_mark: | | :thought_balloon: | -| [`@typescript-eslint/ban-ts-comment`](./docs/rules/ban-ts-comment.md) | Bans `// @ts-` comments from being used | :heavy_check_mark: | | | +| [`@typescript-eslint/ban-ts-comment`](./docs/rules/ban-ts-comment.md) | Bans `// @ts-` comments from being used or requires descriptions after directive | :heavy_check_mark: | | | | [`@typescript-eslint/ban-types`](./docs/rules/ban-types.md) | Bans specific types from being used | :heavy_check_mark: | :wrench: | | | [`@typescript-eslint/class-literal-property-style`](./docs/rules/class-literal-property-style.md) | Ensures that literals on classes are exposed in a consistent style | | :wrench: | | | [`@typescript-eslint/consistent-type-assertions`](./docs/rules/consistent-type-assertions.md) | Enforces consistent usage of type assertions | | | | diff --git a/packages/eslint-plugin/docs/rules/ban-ts-comment.md b/packages/eslint-plugin/docs/rules/ban-ts-comment.md index cb540bc8d09..60e6508d3bd 100644 --- a/packages/eslint-plugin/docs/rules/ban-ts-comment.md +++ b/packages/eslint-plugin/docs/rules/ban-ts-comment.md @@ -1,4 +1,4 @@ -# Bans `// @ts-` comments from being used (`ban-ts-comment`) +# Bans `// @ts-` comments from being used or requires descriptions after directive (`ban-ts-comment`) TypeScript provides several directive comments that can be used to alter how it processes files. Using these to suppress TypeScript Compiler Errors reduces the effectiveness of TypeScript overall. @@ -21,10 +21,11 @@ The configuration looks like this: ```ts interface Options { - 'ts-expect-error'?: boolean; - 'ts-ignore'?: boolean; - 'ts-nocheck'?: boolean; - 'ts-check'?: boolean; + 'ts-expect-error'?: boolean | 'allow-with-description'; + 'ts-ignore'?: boolean | 'allow-with-description'; + 'ts-nocheck'?: boolean | 'allow-with-description'; + 'ts-check'?: boolean | 'allow-with-description'; + minimumDescriptionLength?: number; } const defaultOptions: Options = { @@ -32,9 +33,12 @@ const defaultOptions: Options = { 'ts-ignore': true, 'ts-nocheck': true, 'ts-check': false, + minimumDescriptionLength: 3, }; ``` +### `ts-expect-error`, `ts-ignore`, `ts-nocheck`, `ts-check` directives + A value of `true` for a particular directive means that this rule will report if it finds any usage of said directive. For example, with the defaults above the following patterns are considered warnings: @@ -55,6 +59,50 @@ if (false) { } ``` +### `allow-with-description` + +A value of `'allow-with-description'` for a particular directive means that this rule will report if it finds a directive that does not have a description following the directive (on the same line). + +For example, with `{ 'ts-expect-error': 'allow-with-description' }` the following pattern is considered a warning: + +```ts +if (false) { + // @ts-expect-error + console.log('hello'); +} +``` + +The following pattern is not a warning: + +```ts +if (false) { + // @ts-expect-error: Unreachable code error + console.log('hello'); +} +``` + +### `minimumDescriptionLength` + +Use `minimumDescriptionLength` to set a minimum length for descriptions when using the `allow-with-description` option for a directive. + +For example, with `{ 'ts-expect-error': 'allow-with-description', minimumDescriptionLength: 10 }` the following pattern is considered a warning: + +```ts +if (false) { + // @ts-expect-error: TODO + console.log('hello'); +} +``` + +The following pattern is not a warning: + +```ts +if (false) { + // @ts-expect-error The rationale for this override is described in issue #1337 on GitLab + console.log('hello'); +} +``` + ## When Not To Use It If you want to use all of the TypeScript directives. diff --git a/packages/eslint-plugin/src/rules/ban-ts-comment.ts b/packages/eslint-plugin/src/rules/ban-ts-comment.ts index bcbc02e9a0a..3eccf04ced2 100644 --- a/packages/eslint-plugin/src/rules/ban-ts-comment.ts +++ b/packages/eslint-plugin/src/rules/ban-ts-comment.ts @@ -2,55 +2,96 @@ import { AST_TOKEN_TYPES } from '@typescript-eslint/experimental-utils'; import * as util from '../util'; interface Options { - 'ts-expect-error'?: boolean; - 'ts-ignore'?: boolean; - 'ts-nocheck'?: boolean; - 'ts-check'?: boolean; + 'ts-expect-error'?: boolean | 'allow-with-description'; + 'ts-ignore'?: boolean | 'allow-with-description'; + 'ts-nocheck'?: boolean | 'allow-with-description'; + 'ts-check'?: boolean | 'allow-with-description'; + minimumDescriptionLength?: number; } +export const defaultMinimumDescriptionLength = 3; + const defaultOptions: [Options] = [ { 'ts-expect-error': true, 'ts-ignore': true, 'ts-nocheck': true, 'ts-check': false, + minimumDescriptionLength: defaultMinimumDescriptionLength, }, ]; -type MessageIds = 'tsDirectiveComment'; +type MessageIds = + | 'tsDirectiveComment' + | 'tsDirectiveCommentRequiresDescription'; export default util.createRule<[Options], MessageIds>({ name: 'ban-ts-comment', meta: { type: 'problem', docs: { - description: 'Bans `// @ts-` comments from being used', + description: + 'Bans `// @ts-` comments from being used or requires descriptions after directive', category: 'Best Practices', recommended: 'error', }, messages: { tsDirectiveComment: 'Do not use "// @ts-{{directive}}" because it alters compilation errors.', + tsDirectiveCommentRequiresDescription: + 'Include a description after the "// @ts-{{directive}}" directive to explain why the @ts-{{directive}} is necessary. The description must be {{minimumDescriptionLength}} characters or longer.', }, schema: [ { type: 'object', properties: { 'ts-expect-error': { - type: 'boolean', - default: true, + oneOf: [ + { + type: 'boolean', + default: true, + }, + { + enum: ['allow-with-description'], + }, + ], }, 'ts-ignore': { - type: 'boolean', - default: true, + oneOf: [ + { + type: 'boolean', + default: true, + }, + { + enum: ['allow-with-description'], + }, + ], }, 'ts-nocheck': { - type: 'boolean', - default: true, + oneOf: [ + { + type: 'boolean', + default: true, + }, + { + enum: ['allow-with-description'], + }, + ], }, 'ts-check': { - type: 'boolean', - default: false, + oneOf: [ + { + type: 'boolean', + default: true, + }, + { + enum: ['allow-with-description'], + }, + ], + }, + minimumDescriptionLength: { + type: 'number', + default: defaultMinimumDescriptionLength, }, }, additionalProperties: false, @@ -59,7 +100,7 @@ export default util.createRule<[Options], MessageIds>({ }, defaultOptions, create(context, [options]) { - const tsCommentRegExp = /^\/*\s*@ts-(expect-error|ignore|check|nocheck)/; + const tsCommentRegExp = /^\/*\s*@ts-(expect-error|ignore|check|nocheck)(.*)/; const sourceCode = context.getSourceCode(); return { @@ -71,17 +112,32 @@ export default util.createRule<[Options], MessageIds>({ return; } - const [, directive] = tsCommentRegExp.exec(comment.value) ?? []; + const [, directive, description] = + tsCommentRegExp.exec(comment.value) ?? []; const fullDirective = `ts-${directive}` as keyof Options; - if (options[fullDirective]) { + const option = options[fullDirective]; + if (option === true) { context.report({ data: { directive }, node: comment, messageId: 'tsDirectiveComment', }); } + + if (option === 'allow-with-description') { + const { + minimumDescriptionLength = defaultMinimumDescriptionLength, + } = options; + if (description.trim().length < minimumDescriptionLength) { + context.report({ + data: { directive, minimumDescriptionLength }, + node: comment, + messageId: 'tsDirectiveCommentRequiresDescription', + }); + } + } }); }, }; diff --git a/packages/eslint-plugin/tests/rules/ban-ts-comment.test.ts b/packages/eslint-plugin/tests/rules/ban-ts-comment.test.ts index 89ff1db4a61..2d59e1b315a 100644 --- a/packages/eslint-plugin/tests/rules/ban-ts-comment.test.ts +++ b/packages/eslint-plugin/tests/rules/ban-ts-comment.test.ts @@ -1,5 +1,5 @@ import rule from '../../src/rules/ban-ts-comment'; -import { RuleTester } from '../RuleTester'; +import { RuleTester, noFormat } from '../RuleTester'; const ruleTester = new RuleTester({ parser: '@typescript-eslint/parser', @@ -19,6 +19,23 @@ ruleTester.run('ts-expect-error', rule, { code: '// @ts-expect-error', options: [{ 'ts-expect-error': false }], }, + { + code: '// @ts-expect-error here is why the error is expected', + options: [ + { + 'ts-expect-error': 'allow-with-description', + }, + ], + }, + { + code: '// @ts-expect-error exactly 21 characters', + options: [ + { + 'ts-expect-error': 'allow-with-description', + minimumDescriptionLength: 21, + }, + ], + }, ], invalid: [ { @@ -74,6 +91,39 @@ if (false) { }, ], }, + { + code: '// @ts-expect-error', + options: [ + { + 'ts-expect-error': 'allow-with-description', + }, + ], + errors: [ + { + data: { directive: 'expect-error', minimumDescriptionLength: 3 }, + messageId: 'tsDirectiveCommentRequiresDescription', + line: 1, + column: 1, + }, + ], + }, + { + code: '// @ts-expect-error: TODO', + options: [ + { + 'ts-expect-error': 'allow-with-description', + minimumDescriptionLength: 10, + }, + ], + errors: [ + { + data: { directive: 'expect-error', minimumDescriptionLength: 10 }, + messageId: 'tsDirectiveCommentRequiresDescription', + line: 1, + column: 1, + }, + ], + }, ], }); @@ -91,6 +141,11 @@ ruleTester.run('ts-ignore', rule, { code: '// @ts-ignore', options: [{ 'ts-ignore': false }], }, + { + code: + '// @ts-ignore I think that I am exempted from any need to follow the rules!', + options: [{ 'ts-ignore': 'allow-with-description' }], + }, ], invalid: [ { @@ -154,6 +209,42 @@ if (false) { }, ], }, + { + code: '// @ts-ignore', + options: [{ 'ts-ignore': 'allow-with-description' }], + errors: [ + { + data: { directive: 'ignore', minimumDescriptionLength: 3 }, + messageId: 'tsDirectiveCommentRequiresDescription', + line: 1, + column: 1, + }, + ], + }, + { + code: noFormat`// @ts-ignore `, + options: [{ 'ts-ignore': 'allow-with-description' }], + errors: [ + { + data: { directive: 'ignore', minimumDescriptionLength: 3 }, + messageId: 'tsDirectiveCommentRequiresDescription', + line: 1, + column: 1, + }, + ], + }, + { + code: '// @ts-ignore .', + options: [{ 'ts-ignore': 'allow-with-description' }], + errors: [ + { + data: { directive: 'ignore', minimumDescriptionLength: 3 }, + messageId: 'tsDirectiveCommentRequiresDescription', + line: 1, + column: 1, + }, + ], + }, ], }); @@ -171,6 +262,11 @@ ruleTester.run('ts-nocheck', rule, { code: '// @ts-nocheck', options: [{ 'ts-nocheck': false }], }, + { + code: + '// @ts-nocheck no doubt, people will put nonsense here from time to time just to get the rule to stop reporting, perhaps even long messages with other nonsense in them like other // @ts-nocheck or // @ts-ignore things', + options: [{ 'ts-nocheck': 'allow-with-description' }], + }, ], invalid: [ { @@ -234,6 +330,18 @@ if (false) { }, ], }, + { + code: '// @ts-nocheck', + options: [{ 'ts-nocheck': 'allow-with-description' }], + errors: [ + { + data: { directive: 'nocheck', minimumDescriptionLength: 3 }, + messageId: 'tsDirectiveCommentRequiresDescription', + line: 1, + column: 1, + }, + ], + }, ], }); @@ -251,6 +359,13 @@ ruleTester.run('ts-check', rule, { code: '// @ts-check', options: [{ 'ts-check': false }], }, + { + code: + '// @ts-check with a description and also with a no-op // @ts-ignore', + options: [ + { 'ts-check': 'allow-with-description', minimumDescriptionLength: 3 }, + ], + }, ], invalid: [ { @@ -307,5 +422,17 @@ if (false) { }, ], }, + { + code: '// @ts-ignore', + options: [{ 'ts-ignore': 'allow-with-description' }], + errors: [ + { + data: { directive: 'ignore', minimumDescriptionLength: 3 }, + messageId: 'tsDirectiveCommentRequiresDescription', + line: 1, + column: 1, + }, + ], + }, ], });