From 7fc3b0956340f0268663d3e2a105585a7ecf1584 Mon Sep 17 00:00:00 2001 From: Rafael Santana Date: Tue, 8 May 2018 10:21:51 -0300 Subject: [PATCH] fix(rule): template-conditional-complexity not reporting failures for '[ngIf]' (#611) --- src/templateConditionalComplexityRule.ts | 141 ++++++++++-------- .../templateConditionalComplexityRule.spec.ts | 115 +++++++------- 2 files changed, 142 insertions(+), 114 deletions(-) diff --git a/src/templateConditionalComplexityRule.ts b/src/templateConditionalComplexityRule.ts index bbc4a71c4..c077b3046 100644 --- a/src/templateConditionalComplexityRule.ts +++ b/src/templateConditionalComplexityRule.ts @@ -1,36 +1,33 @@ -import * as Lint from 'tslint'; -import * as ts from 'typescript'; -import * as ast from '@angular/compiler'; +import { AST, ASTWithSource, Binary, BoundDirectivePropertyAst, Lexer, Parser } 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 * as compiler from '@angular/compiler'; -import { Binary } from '@angular/compiler'; +import { BasicTemplateAstVisitor } from './angular/templates/basicTemplateAstVisitor'; -export class Rule extends Lint.Rules.AbstractRule { - public static metadata: Lint.IRuleMetadata = { - ruleName: 'template-conditional-complexity', - type: 'functionality', +export class Rule extends Rules.AbstractRule { + static readonly metadata: IRuleMetadata = { description: "The condition complexity shouldn't exceed a rational limit in a template.", - rationale: 'An important complexity complicates the tests and the maintenance.', + optionExamples: ['true', '[true, 4]'], options: { - type: 'array', items: { type: 'string' }, + maxLength: 2, minLength: 0, - maxLength: 2 + type: 'array' }, - optionExamples: ['true', '[true, 4]'], optionsDescription: 'Determine the maximum number of Boolean operators allowed.', + rationale: 'An important complexity complicates the tests and the maintenance.', + ruleName: 'template-conditional-complexity', + type: 'maintainability', typescriptOnly: true }; - static COMPLEXITY_FAILURE_STRING = "The condition complexity (cost '%s') exceeded the defined limit (cost '%s'). The conditional expression should be moved into the component."; - - static COMPLEXITY_MAX = 3; + static readonly FAILURE_STRING = "The condition complexity (cost '%s') exceeded the defined limit (cost '%s'). The conditional expression should be moved into the component."; + static readonly DEFAULT_MAX_COMPLEXITY = 3; - public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { + apply(sourceFile: SourceFile): RuleFailure[] { return this.applyWithWalker( new NgWalker(sourceFile, this.getOptions(), { templateVisitorCtrl: TemplateConditionalComplexityVisitor @@ -39,53 +36,69 @@ export class Rule extends Lint.Rules.AbstractRule { } } -class TemplateConditionalComplexityVisitor extends BasicTemplateAstVisitor { - visitDirectiveProperty(prop: ast.BoundDirectivePropertyAst, context: BasicTemplateAstVisitor): any { - if (prop.sourceSpan) { - const directive = (prop.sourceSpan).toString(); - - if (directive.startsWith('*ngIf')) { - // extract expression and drop characters new line and quotes - const expr = directive - .split(/\*ngIf\s*=\s*/)[1] - .slice(1, -1) - .replace(/[\n\r]/g, ''); - - const expressionParser = new compiler.Parser(new compiler.Lexer()); - const ast = expressionParser.parseAction(expr, null); - - let complexity = 0; - let conditions: Array = []; - let condition = ast.ast as Binary; - if (condition.operation) { - complexity++; - conditions.push(condition); - } - - while (conditions.length > 0) { - condition = conditions.pop(); - if (condition.operation) { - if (condition.left instanceof Binary) { - complexity++; - conditions.push(condition.left as Binary); - } - - if (condition.right instanceof Binary) { - conditions.push(condition.right as Binary); - } - } - } - const options = this.getOptions(); - const complexityMax: number = options.length ? options[0] : Rule.COMPLEXITY_MAX; - - if (complexity > complexityMax) { - const span = prop.sourceSpan; - let failureConfig: string[] = [String(complexity), 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))); - } - } +export const getFailureMessage = (totalComplexity: number, maxComplexity = Rule.DEFAULT_MAX_COMPLEXITY): string => { + return sprintf(Rule.FAILURE_STRING, totalComplexity, maxComplexity); +}; + +const getTotalComplexity = (ast: AST): number => { + const expr = (ast as ASTWithSource).source.replace(/\s/g, ''); + const expressionParser = new Parser(new Lexer()); + const astWithSource = expressionParser.parseAction(expr, null); + const conditions: Binary[] = []; + let totalComplexity = 0; + let condition = astWithSource.ast as Binary; + + if (condition.operation) { + totalComplexity++; + conditions.push(condition); + } + + while (conditions.length > 0) { + condition = conditions.pop(); + + if (!condition.operation) { + continue; } + + if (condition.left instanceof Binary) { + totalComplexity++; + conditions.push(condition.left); + } + + if (condition.right instanceof Binary) { + conditions.push(condition.right); + } + } + + return totalComplexity; +}; + +class TemplateConditionalComplexityVisitor extends BasicTemplateAstVisitor { + visitDirectiveProperty(prop: BoundDirectivePropertyAst, context: BasicTemplateAstVisitor): any { + this.validateDirective(prop); super.visitDirectiveProperty(prop, context); } + + private validateDirective(prop: BoundDirectivePropertyAst): void { + const { templateName, value } = prop; + + if (templateName !== 'ngIf') { + return; + } + + const maxComplexity: number = this.getOptions()[0] || Rule.DEFAULT_MAX_COMPLEXITY; + const totalComplexity = getTotalComplexity(value); + + if (totalComplexity <= maxComplexity) { + return; + } + + const { + sourceSpan: { + end: { offset: endOffset }, + start: { offset: startOffset } + } + } = prop; + this.addFailureFromStartToEnd(startOffset, endOffset, getFailureMessage(totalComplexity, maxComplexity)); + } } diff --git a/test/templateConditionalComplexityRule.spec.ts b/test/templateConditionalComplexityRule.spec.ts index cd869cc9c..c40e505a3 100644 --- a/test/templateConditionalComplexityRule.spec.ts +++ b/test/templateConditionalComplexityRule.spec.ts @@ -1,122 +1,137 @@ -import { assertSuccess, assertAnnotated } from './testHelper'; +import { getFailureMessage, Rule } from '../src/templateConditionalComplexityRule'; +import { assertAnnotated, assertSuccess } from './testHelper'; -describe('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: \` -
+
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Enter your card details
\` }) class Bar {} `; - assertSuccess('template-conditional-complexity', source); + assertAnnotated({ message: getFailureMessage(5, 4), options: [4], ruleName, source }); }); - it('should work with a lower level of complexity', () => { - let source = ` + it('should fail with a higher level of complexity and a carrier return', () => { + const source = ` @Component({ template: \` -
+
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~ Enter your card details
\` }) class Bar {} `; - assertSuccess('template-conditional-complexity', source); + assertAnnotated({ message: getFailureMessage(5, 3), ruleName, source }); }); - it('should work with a level of complexity customisable', () => { - let source = ` + it('should fail with a higher level of complexity with ng-template', () => { + const source = ` @Component({ template: \` -
+ + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + Enter details + + \` + }) + class Bar {} + `; + assertAnnotated({ message: getFailureMessage(6, 3), ruleName, source }); + }); + }); + + describe('success', () => { + it('should succeed with a lower level of complexity', () => { + const source = ` + @Component({ + template: \` +
Enter your card details
\` }) class Bar {} `; - assertSuccess('template-conditional-complexity', source, [5]); + assertSuccess(ruleName, source); }); - it('should work with a level of complexity customisable', () => { - let source = ` + it('should succeed with a lower level of complexity with separated statements', () => { + const source = ` @Component({ template: \` -
+
Enter your card details
\` }) class Bar {} `; - assertSuccess('template-conditional-complexity', source, [5]); + assertSuccess(ruleName, source); }); - it('should work with something else', () => { - let source = ` + it('should succeed with a level of complexity customizable', () => { + const source = ` @Component({ template: \` -
+
Enter your card details
\` }) class Bar {} `; - assertSuccess('template-conditional-complexity', source, [5]); + assertSuccess(ruleName, source, [5]); }); - }); - describe('failure', () => { - it('should fail with a higher level of complexity', () => { - let source = ` + it('should succeed with a level of complexity customizable', () => { + const source = ` @Component({ template: \` -
- ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +
Enter your card details
\` }) class Bar {} `; - assertAnnotated({ - ruleName: 'template-conditional-complexity', - message: - "The condition complexity (cost '5') exceeded the defined limit (cost '4'). The conditional expression should be moved into the component.", - source, - options: [4] - }); + assertSuccess(ruleName, source, [5]); }); - }); - describe('failure', () => { - it('should fail with a higher level of complexity and a carrier return', () => { - let source = ` + it('should succeed with non-inlined then template', () => { + const source = ` @Component({ template: \` -
- ~~~~~~~~~~~~~~~~~~~~~~~~~~~ +
Enter your card details
+ + thenBlock + + + elseBlock + \` }) class Bar {} `; - assertAnnotated({ - ruleName: 'template-conditional-complexity', - message: - "The condition complexity (cost '5') exceeded the defined limit (cost '3'). The conditional expression should be moved into the component.", - source - }); + assertSuccess(ruleName, source, [5]); }); }); });