From 72a33aa91abc2591f52623ab7e8c2f11b09199a5 Mon Sep 17 00:00:00 2001 From: Pavel Birukov Date: Thu, 30 Jan 2020 16:21:14 +0200 Subject: [PATCH 1/5] fix(eslint-plugin): autofix no-public in explicit-member-accessibility --- .../rules/explicit-member-accessibility.ts | 60 +++++++ .../explicit-member-accessibility.test.ts | 159 ++++++++++++++++++ 2 files changed, 219 insertions(+) diff --git a/packages/eslint-plugin/src/rules/explicit-member-accessibility.ts b/packages/eslint-plugin/src/rules/explicit-member-accessibility.ts index ebfbae27329d..c928fc7ed914 100644 --- a/packages/eslint-plugin/src/rules/explicit-member-accessibility.ts +++ b/packages/eslint-plugin/src/rules/explicit-member-accessibility.ts @@ -1,8 +1,15 @@ import { AST_NODE_TYPES, TSESTree, + AST_TOKEN_TYPES, } from '@typescript-eslint/experimental-utils'; import * as util from '../util'; +import { + ReportFixFunction, + RuleFixer, + RuleFix, +} from '@typescript-eslint/experimental-utils/dist/ts-eslint'; +import { Range } from '../../../typescript-estree/dist/ts-estree/ts-estree'; type AccessibilityLevel = | 'explicit' // require an accessor (including public) @@ -38,6 +45,7 @@ export default util.createRule({ // too opinionated to be recommended recommended: false, }, + fixable: 'code', messages: { missingAccessibility: 'Missing accessibility modifier on {{type}} {{name}}.', @@ -91,6 +99,7 @@ export default util.createRule({ nodeType: string, node: TSESTree.Node, nodeName: string, + fix: ReportFixFunction | null = null, ): void { context.report({ node: node, @@ -99,6 +108,7 @@ export default util.createRule({ type: nodeType, name: nodeName, }, + fix: fix, }); } @@ -140,6 +150,7 @@ export default util.createRule({ nodeType, methodDefinition, methodName, + getUnwantedPublicAccessibilityFixer(methodDefinition), ); } else if (check === 'explicit' && !methodDefinition.accessibility) { reportIssue( @@ -151,6 +162,53 @@ export default util.createRule({ } } + /** + * Creates a fixer that removes a "public" keyword with following spaces + */ + function getUnwantedPublicAccessibilityFixer( + node: + | TSESTree.MethodDefinition + | TSESTree.ClassProperty + | TSESTree.TSParameterProperty, + ): ReportFixFunction { + return function(fixer: RuleFixer): RuleFix { + const tokens = sourceCode.getTokens(node); + let rangeToRemove: Range; + for (let i = 0; i < tokens.length; i++) { + const token = tokens[i]; + if ( + token.type === AST_TOKEN_TYPES.Keyword && + token.value === 'public' + ) { + const commensAfterPublicKeyword = sourceCode.getCommentsAfter( + token, + ); + if (commensAfterPublicKeyword.length) { + // public /* Hi there! */ static foo() + // ^^^^^^^ + rangeToRemove = [ + token.range[0], + commensAfterPublicKeyword[0].range[0], + ]; + break; + } else { + // public static foo() + // ^^^^^^^ + rangeToRemove = [token.range[0], tokens[i + 1].range[0]]; + break; + } + } + } + if (typeof rangeToRemove! === 'undefined') { + throw new Error( + `Couldn't define a range for "public" keyword that is to be deleted`, + ); + } + + return fixer.removeRange(rangeToRemove); + }; + } + /** * Checks if property has an accessibility modifier. * @param classProperty The node representing a ClassProperty. @@ -170,6 +228,7 @@ export default util.createRule({ nodeType, classProperty, propertyName, + getUnwantedPublicAccessibilityFixer(classProperty), ); } else if (propCheck === 'explicit' && !classProperty.accessibility) { reportIssue( @@ -217,6 +276,7 @@ export default util.createRule({ nodeType, node, nodeName, + getUnwantedPublicAccessibilityFixer(node), ); } break; diff --git a/packages/eslint-plugin/tests/rules/explicit-member-accessibility.test.ts b/packages/eslint-plugin/tests/rules/explicit-member-accessibility.test.ts index b5477c062a10..aa50954fef4e 100644 --- a/packages/eslint-plugin/tests/rules/explicit-member-accessibility.test.ts +++ b/packages/eslint-plugin/tests/rules/explicit-member-accessibility.test.ts @@ -706,5 +706,164 @@ class Test { }, ], }, + { + filename: 'test.ts', + code: ` +class Test { + @public + public /*public*/constructor(private foo: string) {} +} + `, + options: [ + { + accessibility: 'no-public', + }, + ], + errors: [ + { + messageId: 'unwantedPublicAccessibility', + line: 3, + column: 3, + }, + ], + output: ` +class Test { + @public + /*public*/constructor(private foo: string) {} +} + `, + }, + { + filename: 'test.ts', + code: ` +class Test { + @public + public foo() {} +} + `, + options: [ + { + accessibility: 'no-public', + }, + ], + errors: [ + { + messageId: 'unwantedPublicAccessibility', + line: 3, + column: 3, + }, + ], + output: ` +class Test { + @public + foo() {} +} + `, + }, + + { + filename: 'test.ts', + code: ` +class Test { + @public + public foo; +} + `, + options: [ + { + accessibility: 'no-public', + }, + ], + errors: [ + { + messageId: 'unwantedPublicAccessibility', + line: 3, + column: 3, + }, + ], + output: ` +class Test { + @public + foo; +} + `, + }, + { + filename: 'test.ts', + code: ` +class Test { + public foo = ""; +} + `, + options: [ + { + accessibility: 'no-public', + }, + ], + errors: [ + { + messageId: 'unwantedPublicAccessibility', + line: 3, + column: 3, + }, + ], + output: ` +class Test { + foo = ""; +} + `, + }, + + { + filename: 'test.ts', + code: ` +class Test { + contructor(public /* Hi there */ readonly foo) +} + `, + options: [ + { + accessibility: 'no-public', + overrides: { parameterProperties: 'no-public' }, + }, + ], + errors: [ + { + messageId: 'unwantedPublicAccessibility', + line: 3, + column: 14, + }, + ], + output: ` +class Test { + contructor(/* Hi there */ readonly foo) +} + `, + }, + { + filename: 'test.ts', + code: ` +class Test { + contructor(public readonly foo: string) +} + `, + options: [ + { + accessibility: 'no-public', + }, + ], + errors: [ + { + messageId: 'unwantedPublicAccessibility', + line: 3, + column: 14, + }, + ], + output: ` +class Test { + contructor(readonly foo: string) +} + `, + }, ], }); From 9ed399c1129153ee914dacbb07e73348686ae5dd Mon Sep 17 00:00:00 2001 From: Pavel Birukov Date: Thu, 30 Jan 2020 16:35:21 +0200 Subject: [PATCH 2/5] fix(eslint-plugin): update documentation --- packages/eslint-plugin/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/eslint-plugin/README.md b/packages/eslint-plugin/README.md index fd24f0e97f50..a543253fa933 100644 --- a/packages/eslint-plugin/README.md +++ b/packages/eslint-plugin/README.md @@ -102,7 +102,7 @@ Pro Tip: For larger codebases you may want to consider splitting our linting int | [`@typescript-eslint/consistent-type-assertions`](./docs/rules/consistent-type-assertions.md) | Enforces consistent usage of type assertions | :heavy_check_mark: | | | | [`@typescript-eslint/consistent-type-definitions`](./docs/rules/consistent-type-definitions.md) | Consistent with type definition either `interface` or `type` | | :wrench: | | | [`@typescript-eslint/explicit-function-return-type`](./docs/rules/explicit-function-return-type.md) | Require explicit return types on functions and class methods | :heavy_check_mark: | | | -| [`@typescript-eslint/explicit-member-accessibility`](./docs/rules/explicit-member-accessibility.md) | Require explicit accessibility modifiers on class properties and methods | | | | +| [`@typescript-eslint/explicit-member-accessibility`](./docs/rules/explicit-member-accessibility.md) | Require explicit accessibility modifiers on class properties and methods | | :wrench: | | | [`@typescript-eslint/explicit-module-boundary-types`](./docs/rules/explicit-module-boundary-types.md) | Require explicit return and argument types on exported functions' and classes' public class methods | | | | | [`@typescript-eslint/member-delimiter-style`](./docs/rules/member-delimiter-style.md) | Require a specific member delimiter style for interfaces and type literals | :heavy_check_mark: | :wrench: | | | [`@typescript-eslint/member-ordering`](./docs/rules/member-ordering.md) | Require a consistent member declaration order | | | | From 0d00da316e985697ddd8d7ff7abb9daf58c144dc Mon Sep 17 00:00:00 2001 From: Pavel Birukov Date: Thu, 30 Jan 2020 17:17:13 +0200 Subject: [PATCH 3/5] fix(eslint-plugin): remove throw statement as untestable --- .../src/rules/explicit-member-accessibility.ts | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/packages/eslint-plugin/src/rules/explicit-member-accessibility.ts b/packages/eslint-plugin/src/rules/explicit-member-accessibility.ts index c928fc7ed914..a2745b2e7209 100644 --- a/packages/eslint-plugin/src/rules/explicit-member-accessibility.ts +++ b/packages/eslint-plugin/src/rules/explicit-member-accessibility.ts @@ -199,13 +199,7 @@ export default util.createRule({ } } } - if (typeof rangeToRemove! === 'undefined') { - throw new Error( - `Couldn't define a range for "public" keyword that is to be deleted`, - ); - } - - return fixer.removeRange(rangeToRemove); + return fixer.removeRange(rangeToRemove!); }; } From bf2bd24b762ea95e54670a8058df03a58f17fa1e Mon Sep 17 00:00:00 2001 From: Pavel Birukov Date: Fri, 31 Jan 2020 10:27:20 +0200 Subject: [PATCH 4/5] fix(eslint-plugin): remove relative imports --- .../src/rules/explicit-member-accessibility.ts | 15 +++++---------- .../rules/explicit-member-accessibility.test.ts | 2 +- 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/packages/eslint-plugin/src/rules/explicit-member-accessibility.ts b/packages/eslint-plugin/src/rules/explicit-member-accessibility.ts index a2745b2e7209..7c49f0d6a318 100644 --- a/packages/eslint-plugin/src/rules/explicit-member-accessibility.ts +++ b/packages/eslint-plugin/src/rules/explicit-member-accessibility.ts @@ -2,14 +2,9 @@ import { AST_NODE_TYPES, TSESTree, AST_TOKEN_TYPES, + TSESLint, } from '@typescript-eslint/experimental-utils'; import * as util from '../util'; -import { - ReportFixFunction, - RuleFixer, - RuleFix, -} from '@typescript-eslint/experimental-utils/dist/ts-eslint'; -import { Range } from '../../../typescript-estree/dist/ts-estree/ts-estree'; type AccessibilityLevel = | 'explicit' // require an accessor (including public) @@ -99,7 +94,7 @@ export default util.createRule({ nodeType: string, node: TSESTree.Node, nodeName: string, - fix: ReportFixFunction | null = null, + fix: TSESLint.ReportFixFunction | null = null, ): void { context.report({ node: node, @@ -170,10 +165,10 @@ export default util.createRule({ | TSESTree.MethodDefinition | TSESTree.ClassProperty | TSESTree.TSParameterProperty, - ): ReportFixFunction { - return function(fixer: RuleFixer): RuleFix { + ): TSESLint.ReportFixFunction { + return function(fixer: TSESLint.RuleFixer): TSESLint.RuleFix { const tokens = sourceCode.getTokens(node); - let rangeToRemove: Range; + let rangeToRemove: TSESLint.AST.Range; for (let i = 0; i < tokens.length; i++) { const token = tokens[i]; if ( diff --git a/packages/eslint-plugin/tests/rules/explicit-member-accessibility.test.ts b/packages/eslint-plugin/tests/rules/explicit-member-accessibility.test.ts index aa50954fef4e..dc12410d7fa7 100644 --- a/packages/eslint-plugin/tests/rules/explicit-member-accessibility.test.ts +++ b/packages/eslint-plugin/tests/rules/explicit-member-accessibility.test.ts @@ -818,7 +818,7 @@ class Test { filename: 'test.ts', code: ` class Test { - contructor(public /* Hi there */ readonly foo) + contructor(public/* Hi there */ readonly foo) } `, options: [ From b1cffdde33cda7101874c3599e4f5d0356ac2a57 Mon Sep 17 00:00:00 2001 From: Pavel Birukov Date: Sat, 1 Feb 2020 20:07:12 +0200 Subject: [PATCH 5/5] fix(eslint-plugin): add tests' output --- .../explicit-member-accessibility.test.ts | 180 ++++++++++++++++++ 1 file changed, 180 insertions(+) diff --git a/packages/eslint-plugin/tests/rules/explicit-member-accessibility.test.ts b/packages/eslint-plugin/tests/rules/explicit-member-accessibility.test.ts index dc12410d7fa7..fe0a09dd4c16 100644 --- a/packages/eslint-plugin/tests/rules/explicit-member-accessibility.test.ts +++ b/packages/eslint-plugin/tests/rules/explicit-member-accessibility.test.ts @@ -344,6 +344,11 @@ export class XXXX { line: 3, }, ], + output: ` +export class XXXX { + public constructor(readonly value: string) {} +} + `, }, { filename: 'test.ts', @@ -354,6 +359,11 @@ export class WithParameterProperty { `, options: [{ accessibility: 'explicit' }], errors: [{ messageId: 'missingAccessibility' }], + output: ` +export class WithParameterProperty { + public constructor(readonly value: string) {} +} + `, }, { filename: 'test.ts', @@ -372,6 +382,11 @@ export class XXXX { }, ], errors: [{ messageId: 'missingAccessibility' }], + output: ` +export class XXXX { + public constructor(readonly samosa: string) {} +} + `, }, { filename: 'test.ts', @@ -387,6 +402,11 @@ class Test { }, ], errors: [{ messageId: 'missingAccessibility' }], + output: ` +class Test { + public constructor(readonly foo: string) {} +} + `, }, { filename: 'test.ts', @@ -409,6 +429,14 @@ class Test { column: 3, }, ], + output: ` +class Test { + x: number + public getX () { + return this.x + } +} + `, }, { filename: 'test.ts', @@ -431,6 +459,14 @@ class Test { column: 3, }, ], + output: ` +class Test { + private x: number + getX () { + return this.x + } +} + `, }, { filename: 'test.ts', @@ -462,6 +498,14 @@ class Test { column: 3, }, ], + output: ` +class Test { + x?: number + getX? () { + return this.x + } +} + `, }, { filename: 'test.ts', @@ -486,6 +530,15 @@ class Test { column: 3, }, ], + output: ` +class Test { + protected name: string + protected foo?: string + getX () { + return this.x + } +} + `, }, { filename: 'test.ts', @@ -510,6 +563,15 @@ class Test { column: 3, }, ], + output: ` +class Test { + protected name: string + foo?: string + getX () { + return this.x + } +} + `, }, { filename: 'test.ts', @@ -534,6 +596,14 @@ class Test { }, ], options: [{ accessibility: 'no-public' }], + output: ` +class Test { + x: number + getX () { + return this.x + } +} + `, }, { filename: 'test.ts', @@ -564,6 +634,20 @@ class Test { }, ], options: [{ overrides: { constructors: 'no-public' } }], + output: ` +class Test { + private x: number; + constructor (x: number) { + this.x = x; + } + get internalValue() { + return this.x; + } + set internalValue(value: number) { + this.x = value; + } +} + `, }, { filename: 'test.ts', @@ -598,6 +682,20 @@ class Test { column: 3, }, ], + output: ` +class Test { + private x: number; + constructor (x: number) { + this.x = x; + } + get internalValue() { + return this.x; + } + set internalValue(value: number) { + this.x = value; + } +} + `, }, { filename: 'test.ts', @@ -621,6 +719,14 @@ class Test { overrides: { parameterProperties: 'no-public' }, }, ], + output: ` +class Test { + constructor(public x: number){} + public foo(): string { + return 'foo'; + } +} + `, }, { filename: 'test.ts', @@ -636,6 +742,11 @@ class Test { column: 3, }, ], + output: ` +class Test { + constructor(public x: number){} +} + `, }, { filename: 'test.ts', @@ -657,6 +768,11 @@ class Test { column: 15, }, ], + output: ` +class Test { + constructor(readonly x: number){} +} + `, }, { filename: 'test.ts', @@ -674,6 +790,7 @@ class Test { column: 14, }, ], + output: 'class Test { x = 2 }', }, { filename: 'test.ts', @@ -694,6 +811,10 @@ class Test { column: 9, }, ], + output: `class Test { + x = 2 + private x = 2 + }`, }, { code: 'class Test { constructor(public ...x: any[]) { }}', @@ -705,6 +826,7 @@ class Test { column: 14, }, ], + output: 'class Test { constructor(public ...x: any[]) { }}', }, { filename: 'test.ts', @@ -865,5 +987,63 @@ class Test { } `, }, + { + filename: 'test.ts', + code: + 'class EnsureWhiteSPaceSpan { public constructor() {}}', + options: [ + { + accessibility: 'no-public', + overrides: { parameterProperties: 'no-public' }, + }, + ], + errors: [ + { + messageId: 'unwantedPublicAccessibility', + line: 1, + column: 30, + }, + ], + output: 'class EnsureWhiteSPaceSpan { constructor() {}}', + }, + { + filename: 'test.ts', + code: + 'class EnsureWhiteSPaceSpan { public /* */ constructor() {}}', + options: [ + { + accessibility: 'no-public', + overrides: { parameterProperties: 'no-public' }, + }, + ], + errors: [ + { + messageId: 'unwantedPublicAccessibility', + line: 1, + column: 30, + }, + ], + output: + 'class EnsureWhiteSPaceSpan { /* */ constructor() {}}', + }, + { + filename: 'test.ts', + code: + 'class EnsureWhiteSPaceSpan { public /* */ constructor() {}}', + options: [ + { + accessibility: 'no-public', + overrides: { parameterProperties: 'no-public' }, + }, + ], + errors: [ + { + messageId: 'unwantedPublicAccessibility', + line: 1, + column: 30, + }, + ], + output: 'class EnsureWhiteSPaceSpan { /* */ constructor() {}}', + }, ], });