From 5e34f4183de2d1f70dee3712297e3aafb046a416 Mon Sep 17 00:00:00 2001 From: Rafael Santana Date: Sun, 6 May 2018 17:55:29 -0300 Subject: [PATCH] fix(rule): some cases not being reported by no-output-rename rule (#614) --- src/noInputRenameRule.ts | 6 +-- src/noOutputRenameRule.ts | 56 +++++++++++++-------- test/noOutputRenameRule.spec.ts | 86 ++++++++++++++++++++++++--------- 3 files changed, 101 insertions(+), 47 deletions(-) diff --git a/src/noInputRenameRule.ts b/src/noInputRenameRule.ts index c6b30329b..e7c5bb4c3 100644 --- a/src/noInputRenameRule.ts +++ b/src/noInputRenameRule.ts @@ -30,10 +30,10 @@ export const getFailureMessage = (className: string, propertyName: string): stri }; export class InputMetadataWalker extends NgWalker { - private directiveSelectors: DirectiveMetadata['selector'][]; + private directiveSelectors: ReadonlySet; protected visitNgDirective(metadata: DirectiveMetadata): void { - this.directiveSelectors = (metadata.selector || '').replace(/[\[\]\s]/g, '').split(','); + this.directiveSelectors = new Set((metadata.selector || '').replace(/[\[\]\s]/g, '').split(',')); super.visitNgDirective(metadata); } @@ -43,7 +43,7 @@ export class InputMetadataWalker extends NgWalker { } private canPropertyBeAliased(propertyAlias: string, propertyName: string): boolean { - return !!(this.directiveSelectors && this.directiveSelectors.indexOf(propertyAlias) !== -1 && propertyAlias !== propertyName); + return !!(this.directiveSelectors && this.directiveSelectors.has(propertyAlias) && propertyAlias !== propertyName); } private validateInput(property: ts.PropertyDeclaration, input: ts.Decorator, args: string[]) { diff --git a/src/noOutputRenameRule.ts b/src/noOutputRenameRule.ts index 581f0d4ea..9fab91b17 100644 --- a/src/noOutputRenameRule.ts +++ b/src/noOutputRenameRule.ts @@ -1,39 +1,55 @@ -import * as Lint from 'tslint'; -import * as ts from 'typescript'; -import { sprintf } from 'sprintf-js'; +import { IRuleMetadata, RuleFailure, Rules } from 'tslint/lib'; +import { Decorator, PropertyDeclaration, SourceFile } from 'typescript/lib/typescript'; +import { DirectiveMetadata } from './angular/metadata'; import { NgWalker } from './angular/ngWalker'; -export class Rule extends Lint.Rules.AbstractRule { - public static metadata: Lint.IRuleMetadata = { - ruleName: 'no-output-rename', - type: 'maintainability', +export class Rule extends Rules.AbstractRule { + static readonly metadata: IRuleMetadata = { description: 'Disallows renaming directive outputs by providing a string to the decorator.', descriptionDetails: 'See more at https://angular.io/styleguide#style-05-13.', - rationale: 'Two names for the same property (one private, one public) is inherently confusing.', options: null, optionsDescription: 'Not configurable.', + rationale: 'Two names for the same property (one private, one public) is inherently confusing.', + ruleName: 'no-output-rename', + type: 'maintainability', typescriptOnly: true }; - static FAILURE_STRING: string = 'In the class "%s", the directive output ' + - 'property "%s" should not be renamed.' + - 'Please, consider the following use "@Output() %s = new EventEmitter();"'; + static readonly FAILURE_STRING = '@Outputs should not be renamed'; - public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { + apply(sourceFile: SourceFile): RuleFailure[] { return this.applyWithWalker(new OutputMetadataWalker(sourceFile, this.getOptions())); } } +export const getFailureMessage = (): string => { + return Rule.FAILURE_STRING; +}; + export class OutputMetadataWalker extends NgWalker { - protected visitNgOutput(property: ts.PropertyDeclaration, output: ts.Decorator, args: string[]) { - let className = (property).parent.name.text; - let memberName = (property.name).text; - if (args.length !== 0 && memberName !== args[0]) { - let failureConfig: string[] = [className, memberName, memberName]; - failureConfig.unshift(Rule.FAILURE_STRING); - this.addFailure(this.createFailure(property.getStart(), property.getWidth(), sprintf.apply(this, failureConfig))); - } + private directiveSelectors: ReadonlySet; + visitNgDirective(metadata: DirectiveMetadata): void { + this.directiveSelectors = new Set((metadata.selector || '').replace(/[\[\]\s]/g, '').split(',')); + super.visitNgDirective(metadata); + } + + protected visitNgOutput(property: PropertyDeclaration, output: Decorator, args: string[]) { + this.validateOutput(property, output, args); super.visitNgOutput(property, output, args); } + + private canPropertyBeAliased(propertyAlias: string, propertyName: string): boolean { + return !!(this.directiveSelectors && this.directiveSelectors.has(propertyAlias) && propertyAlias !== propertyName); + } + + private validateOutput(property: PropertyDeclaration, output: Decorator, args: string[]) { + const propertyName = property.name.getText(); + + if (args.length === 0 || this.canPropertyBeAliased(args[0], propertyName)) { + return; + } + + this.addFailureAtNode(property, getFailureMessage()); + } } diff --git a/test/noOutputRenameRule.spec.ts b/test/noOutputRenameRule.spec.ts index 5f89af974..1177dd07a 100644 --- a/test/noOutputRenameRule.spec.ts +++ b/test/noOutputRenameRule.spec.ts @@ -1,41 +1,79 @@ -import { assertSuccess, assertAnnotated } from './testHelper'; +import { getFailureMessage, Rule } from '../src/noOutputRenameRule'; +import { assertAnnotated, assertSuccess } from './testHelper'; -describe('no-output-rename', () => { - describe('invalid directive output property', () => { - it('should fail, when a directive output property is renamed', () => { - let source = ` - class ButtonComponent { - @Output('changeEvent') change = new EventEmitter(); - ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +const { + metadata: { ruleName } +} = Rule; + +describe(ruleName, () => { + describe('failure', () => { + it('should fail when a directive output property is renamed', () => { + const source = ` + @Component({ + template: 'test' + }) + class TestComponent { + @Output('onChange') change = new EventEmitter(); + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ } `; assertAnnotated({ - ruleName: 'no-output-rename', - message: - 'In the class "ButtonComponent", the directive output property "change" should not be renamed.' + - 'Please, consider the following use "@Output() change = new EventEmitter();"', + message: getFailureMessage(), + ruleName, source }); }); - }); - describe('valid directive output property', () => { - it('should succeed, when a directive output property is properly used', () => { - let source = ` - class ButtonComponent { - @Output() change = new EventEmitter(); + it('should fail when a directive output property is renamed and its name is strictly equal to the property', () => { + const source = ` + @Component({ + template: 'test' + }) + class TestComponent { + @Output('change') change = new EventEmitter(); + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ } `; - assertSuccess('no-output-rename', source); + assertAnnotated({ message: getFailureMessage(), ruleName, source }); }); - it('should succeed, when a directive output property rename is the same as the property name', () => { - let source = ` - class ButtonComponent { - @Output('change') change = new EventEmitter(); + it("should fail when the directive's selector matches exactly both property name and the alias", () => { + const source = ` + @Directive({ + selector: '[test], foo' + }) + class TestDirective { + @Output('test') test = new EventEmitter(); + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ } `; - assertSuccess('no-output-rename', source); + assertAnnotated({ message: getFailureMessage(), ruleName, source }); + }); + }); + + describe('success', () => { + it('should succeed when a directive output property is properly used', () => { + const source = ` + @Component({ + template: 'test' + }) + class TestComponent { + @Output() change = new EventEmitter(); + } + `; + assertSuccess(ruleName, source); + }); + + it("should succeed when the directive's selector is also an output property", () => { + const source = ` + @Directive({ + selector: '[foo], test' + }) + class TestDirective { + @Output('foo') bar = new EventEmitter(); + } + `; + assertSuccess(ruleName, source); }); }); });