From 5d5e21d4e8cff3b4bb1afc88d23ac6e34fe28c83 Mon Sep 17 00:00:00 2001 From: Rafael Santana Date: Mon, 30 Apr 2018 12:06:31 -0300 Subject: [PATCH] feat(rule): add prefer-inline-decorator rule (#586) --- src/preferInlineDecoratorRule.ts | 91 +++++++++++ test/preferInlineDecoratorRule.spec.ts | 201 +++++++++++++++++++++++++ 2 files changed, 292 insertions(+) create mode 100644 src/preferInlineDecoratorRule.ts create mode 100644 test/preferInlineDecoratorRule.spec.ts diff --git a/src/preferInlineDecoratorRule.ts b/src/preferInlineDecoratorRule.ts new file mode 100644 index 000000000..a0b5bf501 --- /dev/null +++ b/src/preferInlineDecoratorRule.ts @@ -0,0 +1,91 @@ +import { IOptions, IRuleMetadata, Replacement, RuleFailure, Rules } from 'tslint/lib'; +import { isSameLine } from 'tsutils'; +import { Decorator, Node, PropertyAccessExpression, SourceFile } from 'typescript'; + +import { NgWalker } from './angular/ngWalker'; +import { getDecoratorName } from './util/utils'; + +export class Rule extends Rules.AbstractRule { + static readonly metadata: IRuleMetadata = { + description: 'Ensures that decorators are on the same line as the property/method it decorates.', + descriptionDetails: 'See more at https://angular.io/guide/styleguide#style-05-12.', + hasFix: true, + optionExamples: ['true', '[true, "HostListener"]'], + options: { + items: { + type: 'string' + }, + type: 'array' + }, + optionsDescription: 'A list of blacklisted decorators.', + rationale: 'Placing the decorator on the same line usually makes for shorter code and still easily identifies the property/method.', + ruleName: 'prefer-inline-decorator', + type: 'style', + typescriptOnly: true + }; + + static readonly FAILURE_STRING = 'Consider placing decorators on the same line as the property/method it decorates'; + + apply(sourceFile: SourceFile): RuleFailure[] { + return this.applyWithWalker(new PreferInlineDecoratorWalker(sourceFile, this.getOptions())); + } +} + +type DecoratorKeys = + | 'ContentChild' + | 'ContentChildren' + | 'HostBinding' + | 'HostListener' + | 'Input' + | 'Output' + | 'ViewChild' + | 'ViewChildren'; + +export const decoratorKeys = new Set([ + 'ContentChild', + 'ContentChildren', + 'HostBinding', + 'HostListener', + 'Input', + 'Output', + 'ViewChild', + 'ViewChildren' +]); + +export class PreferInlineDecoratorWalker extends NgWalker { + private readonly blacklistedDecorators: typeof decoratorKeys; + + constructor(source: SourceFile, options: IOptions) { + super(source, options); + this.blacklistedDecorators = new Set(options.ruleArguments.slice(1)); + } + + protected visitMethodDecorator(decorator: Decorator) { + this.validateDecorator(decorator, decorator.parent); + super.visitMethodDecorator(decorator); + } + + protected visitPropertyDecorator(decorator: Decorator) { + this.validateDecorator(decorator, decorator.parent); + super.visitPropertyDecorator(decorator); + } + + private validateDecorator(decorator: Decorator, property: Node) { + const decoratorName: DecoratorKeys = getDecoratorName(decorator); + const isDecoratorBlacklisted = this.blacklistedDecorators.has(decoratorName); + + if (isDecoratorBlacklisted) { + return; + } + + const decoratorStartPos = decorator.getStart(); + const propertyStartPos = (property as PropertyAccessExpression).name.getStart(); + + if (isSameLine(this.getSourceFile(), decoratorStartPos, propertyStartPos)) { + return; + } + + const fix = Replacement.deleteFromTo(decorator.getEnd(), propertyStartPos - 1); + this.addFailureAt(decoratorStartPos, property.getWidth(), Rule.FAILURE_STRING, fix); + } +} diff --git a/test/preferInlineDecoratorRule.spec.ts b/test/preferInlineDecoratorRule.spec.ts new file mode 100644 index 000000000..6f5a1a1e9 --- /dev/null +++ b/test/preferInlineDecoratorRule.spec.ts @@ -0,0 +1,201 @@ +import { expect } from 'chai'; +import { Replacement } from 'tslint/lib'; + +import { decoratorKeys, Rule } from '../src/preferInlineDecoratorRule'; +import { assertFailure, assertFailures, assertSuccess, IExpectedFailure } from './testHelper'; + +const { + FAILURE_STRING, + metadata: { ruleName } +} = Rule; +const className = 'Test'; + +describe(ruleName, () => { + describe('failure', () => { + const expectedFailure: IExpectedFailure = { + endPosition: { + character: 35, + line: 3 + }, + message: FAILURE_STRING, + startPosition: { + character: 14, + line: 2 + } + }; + + decoratorKeys.forEach(decoratorKey => { + describe(decoratorKey, () => { + it('should fail when a property does not start on the same line as the decoratorKey', () => { + const source = ` + class ${className} { + @${decoratorKey}('childTest') + childTest: ChildTest; + } + `; + assertFailure(ruleName, source, expectedFailure); + }); + + it('should fail and apply proper replacements when a property does not start on the same line as the decoratorKey', () => { + const source = ` + class ${className} { + @${decoratorKey}('childTest') + childTest: ChildTest; + } + `; + const failures = assertFailure(ruleName, source, expectedFailure); + const replacement = Replacement.applyFixes(source, failures.map(f => f.getFix())); + + expect(replacement).to.eq(` + class ${className} { + @${decoratorKey}('childTest') childTest: ChildTest; + } + `); + }); + }); + }); + + describe('blacklist', () => { + it('should fail when a property does not start on the same line as the decoratorKey and is not present on blacklist options', () => { + const [firstDecorator, ...restDecorators] = Array.from(decoratorKeys); + const source = ` + class ${className} { + @${firstDecorator}() + test = new EventEmitter(); + } + `; + assertFailure( + ruleName, + source, + { + endPosition: { + character: 44, + line: 3 + }, + message: FAILURE_STRING, + startPosition: { + character: 12, + line: 2 + } + }, + [true, restDecorators] + ); + }); + }); + + it('should fail when there are multiple properties that does not start on the same line as the decoratorKey', () => { + const source = ` + class ${className} { + @Input('childTest') + childTest: ChildTest; + @Input('foo') + foo: Foo; + } + `; + assertFailures(ruleName, source, [ + { + endPosition: { + character: 31, + line: 3 + }, + message: FAILURE_STRING, + startPosition: { + character: 10, + line: 2 + } + }, + { + endPosition: { + character: 19, + line: 5 + }, + message: FAILURE_STRING, + startPosition: { + character: 10, + line: 4 + } + } + ]); + }); + }); + + describe('success', () => { + decoratorKeys.forEach(decoratorKey => { + describe(decoratorKey, () => { + it('should succeed when a property starts and ends on the same line as the decoratorKey', () => { + const source = ` + class ${className} { + @${decoratorKey}('childTest') childTest123: ChildTest; + } + `; + assertSuccess(ruleName, source); + }); + + it('should succeed when a property starts on the same line as the decoratorKey and ends on another line', () => { + const source = ` + class ${className} { + @${decoratorKey}('childTest') childTest123: ChildTest = + veryVeryVeryVeryVeryVeryVeryLongDefaultVariable; + } + `; + assertSuccess(ruleName, source); + }); + }); + }); + + describe('blacklist', () => { + it('should succeed when a property starts on another line and is present on blacklist options', () => { + const [firstDecorator] = Array.from(decoratorKeys); + const source = ` + class ${className} { + @${firstDecorator}() + test = new EventEmitter(); + } + `; + assertSuccess(ruleName, source, [true, firstDecorator]); + }); + }); + + describe('special cases', () => { + it('should succeed when getters starts on the same line as the decoratorKey and ends on another line', () => { + const source = ` + class ${className} { + @Input() get test(): string { + return this._test; + } + private _test: string; + } + `; + assertSuccess(ruleName, source); + }); + + it('should succeed when setters starts on the same line as the decoratorKey and ends on another line', () => { + const source = ` + class ${className} { + @Input() set test(value: string) { + this._test = value; + } + private _test: string; + } + `; + assertSuccess(ruleName, source); + }); + + it('should succeed for getters and setters on another line', () => { + const source = ` + class ${className} { + @Input() + get test(): string { + return this._test; + } + set test(value: string) { + this._test = value; + } + private _test: string; + } + `; + assertSuccess(ruleName, source); + }); + }); + }); +});