From fedd331707e7112b91757083b1b0f830ef7a995f Mon Sep 17 00:00:00 2001 From: Rafael Santana Date: Mon, 7 May 2018 07:07:22 -0300 Subject: [PATCH] fix(rule): template-cyclomatic-complexity not reporting failures for '[ngForOf]' and '[ngIf]' (#612) --- src/templateCyclomaticComplexityRule.ts | 76 ++++++------- test/templateCyclomaticComplexityRule.spec.ts | 102 +++++++++++++----- 2 files changed, 114 insertions(+), 64 deletions(-) diff --git a/src/templateCyclomaticComplexityRule.ts b/src/templateCyclomaticComplexityRule.ts index 4db8002d8..5e86c52ff 100644 --- a/src/templateCyclomaticComplexityRule.ts +++ b/src/templateCyclomaticComplexityRule.ts @@ -1,35 +1,34 @@ -import * as Lint from 'tslint'; -import * as ts from 'typescript'; -import * as ast from '@angular/compiler'; +import { BoundDirectivePropertyAst } from '@angular/compiler'; import { sprintf } from 'sprintf-js'; -import { BasicTemplateAstVisitor } from './angular/templates/basicTemplateAstVisitor'; +import { IRuleMetadata, RuleFailure, Rules } from 'tslint/lib'; +import { SourceFile } from 'typescript/lib/typescript'; import { NgWalker } from './angular/ngWalker'; +import { BasicTemplateAstVisitor } from './angular/templates/basicTemplateAstVisitor'; -export class Rule extends Lint.Rules.AbstractRule { - public static metadata: Lint.IRuleMetadata = { - ruleName: 'template-cyclomatic-complexity', - type: 'functionality', +export class Rule extends Rules.AbstractRule { + static readonly metadata: IRuleMetadata = { description: "Checks cyclomatic complexity against a specified limit. It is a quantitative measure of the number of linearly independent paths through a program's source code", - rationale: 'Cyclomatic complexity over some threshold indicates that the logic should be moved outside the template.', + optionExamples: ['true', '[true, 6]'], options: { - type: 'array', items: { type: 'string' }, + maxLength: 2, minLength: 0, - maxLength: 2 + type: 'array' }, - optionExamples: ['true', '[true, 6]'], optionsDescription: 'Determine the maximum number of the cyclomatic complexity.', + rationale: 'Cyclomatic complexity over some threshold indicates that the logic should be moved outside the template.', + ruleName: 'template-cyclomatic-complexity', + type: 'functionality', typescriptOnly: true }; - static COMPLEXITY_FAILURE_STRING = "The cyclomatic complexity exceeded the defined limit (cost '%s'). Your template should be refactored."; - - static COMPLEXITY_MAX = 5; + static readonly FAILURE_STRING = "The cyclomatic complexity exceeded the defined limit (cost '%s'). Your template should be refactored."; + static readonly DEFAULT_MAX_COMPLEXITY = 5; - public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { + apply(sourceFile: SourceFile): RuleFailure[] { return this.applyWithWalker( new NgWalker(sourceFile, this.getOptions(), { templateVisitorCtrl: TemplateConditionalComplexityVisitor @@ -38,33 +37,38 @@ export class Rule extends Lint.Rules.AbstractRule { } } +export const getFailureMessage = (maxComplexity = Rule.DEFAULT_MAX_COMPLEXITY): string => { + return sprintf(Rule.FAILURE_STRING, maxComplexity); +}; + class TemplateConditionalComplexityVisitor extends BasicTemplateAstVisitor { - complexity = 0; + totalComplexity = 0; - visitDirectiveProperty(prop: ast.BoundDirectivePropertyAst, context: BasicTemplateAstVisitor): any { - if (prop.sourceSpan) { - const directive = (prop.sourceSpan).toString(); + visitDirectiveProperty(prop: BoundDirectivePropertyAst, context: any): any { + this.validateDirective(prop); + super.visitDirectiveProperty(prop, context); + } - if ( - directive.startsWith('*ngFor') || - directive.startsWith('*ngIf') || - directive.startsWith('*ngSwitchCase') || - directive.startsWith('*ngSwitchDefault') - ) { - this.complexity++; - } + private validateDirective(prop: BoundDirectivePropertyAst): void { + const pattern = /^ng(ForOf|If|Switch(Case|Default))$/; + const { templateName } = prop; + + if (pattern.test(templateName)) { + this.totalComplexity++; } - const options = this.getOptions(); - const complexityMax: number = options.length ? options[0] : Rule.COMPLEXITY_MAX; + const maxComplexity: number = this.getOptions()[0] || Rule.DEFAULT_MAX_COMPLEXITY; - if (this.complexity > complexityMax) { - const span = prop.sourceSpan; - let failureConfig: string[] = [String(complexityMax)]; - failureConfig.unshift(Rule.COMPLEXITY_FAILURE_STRING); - this.addFailure(this.createFailure(span.start.offset, span.end.offset - span.start.offset, sprintf.apply(this, failureConfig))); + if (this.totalComplexity <= maxComplexity) { + return; } - super.visitDirectiveProperty(prop, context); + const { + sourceSpan: { + end: { offset: endOffset }, + start: { offset: startOffset } + } + } = prop; + this.addFailureFromStartToEnd(startOffset, endOffset, getFailureMessage(maxComplexity)); } } diff --git a/test/templateCyclomaticComplexityRule.spec.ts b/test/templateCyclomaticComplexityRule.spec.ts index f453ff26a..b0d4bf91c 100644 --- a/test/templateCyclomaticComplexityRule.spec.ts +++ b/test/templateCyclomaticComplexityRule.spec.ts @@ -1,77 +1,123 @@ -import { assertSuccess, assertAnnotated } from './testHelper'; +import { getFailureMessage, Rule } from '../src/templateCyclomaticComplexityRule'; +import { assertAnnotated, assertSuccess } from './testHelper'; -describe('cyclomatic complexity', () => { - describe('success', () => { - it('should work with a lower level of complexity', () => { - let source = ` +const { + metadata: { ruleName } +} = Rule; + +describe(ruleName, () => { + describe('failure', () => { + it('should fail with a higher level of complexity', () => { + const source = ` @Component({ template: \`
-
  • - {{ person.name }} +
    +
    {{ person.name }}
    +
    - - - + + + + ~~~~~~~~~~~~~~~~~~~~~~~~~~ +
    -
    • +
    \` }) class Bar {} `; - assertSuccess('template-cyclomatic-complexity', source); + assertAnnotated({ + message: getFailureMessage(), + ruleName, + source + }); }); - it('should work with a higher level of complexity', () => { - let source = ` + it('should fail with a higher level of complexity using directives with ng-template', () => { + const source = ` @Component({ template: \` +
    + + + {{ person.name }} + + + + something here + +
    -
  • +
    {{ person.name }}
    +
    + ~~~~~~~~~~~~~~~~~~~~~
    -
    • +
    \` }) class Bar {} `; - assertSuccess('template-cyclomatic-complexity', source, [7]); + assertAnnotated({ + message: getFailureMessage(6), + options: [6], + ruleName, + source + }); }); }); - describe('failure', () => { - it('should fail with a higher level of complexity', () => { - let source = ` + describe('success', () => { + it('should work with a lower level of complexity', () => { + const source = ` + @Component({ + template: \` +
    +
    + {{ person.name }} +
    + + + +
    +
    +
    + \` + }) + class Bar {} + `; + assertSuccess(ruleName, source); + }); + + it('should work with a higher level of complexity', () => { + const source = ` @Component({ template: \`
    -
  • +
    {{ person.name }}
    +
    - ~~~~~~~~~~~~~~~~~~~~~~~~~~
    -
    • +
    \` }) class Bar {} `; - assertAnnotated({ - ruleName: 'template-cyclomatic-complexity', - message: "The cyclomatic complexity exceeded the defined limit (cost '5'). Your template should be refactored.", - source - }); + assertSuccess(ruleName, source, [7]); }); }); });