diff --git a/src/angular/ngWalker.ts b/src/angular/ngWalker.ts index 8e6b93d38..639e7e3df 100644 --- a/src/angular/ngWalker.ts +++ b/src/angular/ngWalker.ts @@ -47,12 +47,6 @@ export class NgWalker extends Lint.RuleWalker { cssVisitorCtrl: BasicCssAstVisitor }, this._config || {}); - this._config = Object.assign({ - templateVisitorCtrl: BasicTemplateAstVisitor, - expressionVisitorCtrl: RecursiveAngularExpressionVisitor, - cssVisitorCtrl: BasicCssAstVisitor - }, this._config || {}); - // this._config = ngWalkerFactoryUtils.normalizeConfig(this._config); } visitClassDeclaration(declaration: ts.ClassDeclaration) { diff --git a/src/angularWhitespaceRule.ts b/src/angularWhitespaceRule.ts index ec6eb6390..469344df0 100644 --- a/src/angularWhitespaceRule.ts +++ b/src/angularWhitespaceRule.ts @@ -2,10 +2,10 @@ import * as Lint from 'tslint'; import * as ts from 'typescript'; import {NgWalker} from './angular/ngWalker'; import * as ast from '@angular/compiler'; -import { BasicTemplateAstVisitor } from './angular/templates/basicTemplateAstVisitor'; -import { ExpTypes } from './angular/expressionTypes'; -import { Config } from './angular/config'; -import { RecursiveAngularExpressionVisitor } from './angular/templates/recursiveAngularExpressionVisitor'; +import {BasicTemplateAstVisitor} from './angular/templates/basicTemplateAstVisitor'; +import {ExpTypes} from './angular/expressionTypes'; +import {Config} from './angular/config'; +import {RecursiveAngularExpressionVisitor} from './angular/templates/recursiveAngularExpressionVisitor'; const InterpolationOpen = Config.interpolation[0]; const InterpolationClose = Config.interpolation[1]; @@ -13,6 +13,9 @@ const InterpolationNoWhitespaceRe = new RegExp(`${InterpolationOpen}\\S(.*?)\\S$ `\\s(.*?)\\S${InterpolationClose}|${InterpolationOpen}\\S(.*?)\\s${InterpolationClose}`); const InterpolationExtraWhitespaceRe = new RegExp(`${InterpolationOpen}\\s\\s(.*?)\\s${InterpolationClose}|${InterpolationOpen}\\s(.*?)\\s\\s${InterpolationClose}`); +const SemicolonNoWhitespaceNotInSimpleQuoteRe = new RegExp(/;\S(?![^']*')/); +const SemicolonNoWhitespaceNotInDoubleQuoteRe = new RegExp(/;\S(?![^"]*")/); + const getReplacements = (text: ast.BoundTextAst, absolutePosition: number) => { const expr: string = (text.value as any).source; @@ -27,7 +30,16 @@ const getReplacements = (text: ast.BoundTextAst, absolutePosition: number) => { ]; }; -type Option = 'check-interpolation' | 'check-pipe'; + +const getSemicolonReplacements = (text: ast.BoundDirectivePropertyAst, absolutePosition: number) => { + + return [ + new Lint.Replacement(absolutePosition, 1, '; ') + ]; + +}; + +type Option = 'check-interpolation' | 'check-pipe' | 'check-semicolon'; interface ConfigurableVisitor { getOption(): Option; @@ -66,9 +78,52 @@ class InterpolationWhitespaceVisitor extends BasicTemplateAstVisitor implements } } +class SemicolonTemplateVisitor extends BasicTemplateAstVisitor implements ConfigurableVisitor { + + visitDirectiveProperty(prop: ast.BoundDirectivePropertyAst, context: BasicTemplateAstVisitor): any { + + + if (prop.sourceSpan) { + const directive = (prop.sourceSpan).toString(); + const rawExpression = directive.split('=')[1].trim(); + const expr = rawExpression.substring(1, rawExpression.length - 1).trim(); + + const doubleQuote = rawExpression.substring(0, 1).indexOf('\"') === 0; + + // Note that will not be reliable for different interpolation symbols + let error = null; + + if (doubleQuote && SemicolonNoWhitespaceNotInSimpleQuoteRe.test(expr)) { + error = 'Missing whitespace after semicolon; expecting \'; expr\''; + const internalStart = expr.search(SemicolonNoWhitespaceNotInSimpleQuoteRe) + 1; + const start = prop.sourceSpan.start.offset + internalStart + directive.length - directive.split('=')[1].trim().length + 1; + const absolutePosition = context.getSourcePosition(start - 1); + return context.addFailure(context.createFailure(start, 2, + error, getSemicolonReplacements(prop, absolutePosition)) + ); + } else if (!doubleQuote && SemicolonNoWhitespaceNotInDoubleQuoteRe.test(expr)) { + error = 'Missing whitespace after semicolon; expecting \'; expr\''; + const internalStart = expr.search(SemicolonNoWhitespaceNotInDoubleQuoteRe) + 1; + const start = prop.sourceSpan.start.offset + internalStart + directive.length - directive.split('=')[1].trim().length + 1; + const absolutePosition = context.getSourcePosition(start - 1); + return context.addFailure(context.createFailure(start, 2, + error, getSemicolonReplacements(prop, absolutePosition)) + ); + } + } + } + + getOption(): Option { + return 'check-semicolon'; + } + +} + + class WhitespaceTemplateVisitor extends BasicTemplateAstVisitor { private visitors: (BasicTemplateAstVisitor & ConfigurableVisitor)[] = [ - new InterpolationWhitespaceVisitor(this.getSourceFile(), this.getOptions(), this.context, this.templateStart) + new InterpolationWhitespaceVisitor(this.getSourceFile(), this.getOptions(), this.context, this.templateStart), + new SemicolonTemplateVisitor(this.getSourceFile(), this.getOptions(), this.context, this.templateStart) ]; visitBoundText(text: ast.BoundTextAst, context: any): any { @@ -80,8 +135,21 @@ class WhitespaceTemplateVisitor extends BasicTemplateAstVisitor { .forEach(f => this.addFailure(f)); super.visitBoundText(text, context); } + + visitDirectiveProperty(prop: ast.BoundDirectivePropertyAst, context: any): any { + const options = this.getOptions(); + this.visitors + .filter(v => options.indexOf(v.getOption()) >= 0) + .map(v => v.visitDirectiveProperty(prop, this)) + .filter(f => !!f) + .forEach(f => this.addFailure(f)); + super.visitDirectiveProperty(prop, context); + } + + } + /* Expression visitors */ class PipeWhitespaceVisitor extends RecursiveAngularExpressionVisitor implements ConfigurableVisitor { @@ -123,8 +191,8 @@ class PipeWhitespaceVisitor extends RecursiveAngularExpressionVisitor implements if (replacements.length) { context.addFailure( context.createFailure(ast.exp.span.end - 1, 3, - 'The pipe operator should be surrounded by one space on each side, i.e. " | ".', - replacements) + 'The pipe operator should be surrounded by one space on each side, i.e. " | ".', + replacements) ); } super.visitPipe(ast, context); @@ -163,16 +231,19 @@ export class Rule extends Lint.Rules.AbstractRule { description: `Ensures the proper formatting of Angular expressions.`, rationale: `Having whitespace in the right places in an Angular expression makes the template more readable.`, optionsDescription: Lint.Utils.dedent` - One argument may be optionally provided: - * \`"check-interpolation"\` checks for whitespace before and after the interpolation characters`, + Arguments may be optionally provided: + * \`"check-interpolation"\` checks for whitespace before and after the interpolation characters + * \`"check-pipe"\` checks for whitespace before and after a pipe + * \`"check-semicolon"\` checks for whitespace after semicolon`, + options: { type: 'array', items: { type: 'string', - enum: ['check-interpolation'], + enum: ['check-interpolation', 'check-pipe', 'check-semicolon'], }, minLength: 0, - maxLength: 1, + maxLength: 3, }, optionExamples: ['[true, "check-interpolation"]'], typescriptOnly: true, @@ -180,10 +251,10 @@ export class Rule extends Lint.Rules.AbstractRule { public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { return this.applyWithWalker( - new NgWalker(sourceFile, - this.getOptions(), { - templateVisitorCtrl: WhitespaceTemplateVisitor, - expressionVisitorCtrl: TemplateExpressionVisitor - })); + new NgWalker(sourceFile, + this.getOptions(), { + templateVisitorCtrl: WhitespaceTemplateVisitor, + expressionVisitorCtrl: TemplateExpressionVisitor, + })); } } diff --git a/test/angularWhitespaceRule.spec.ts b/test/angularWhitespaceRule.spec.ts index 6958032ff..cb7304ae0 100644 --- a/test/angularWhitespaceRule.spec.ts +++ b/test/angularWhitespaceRule.spec.ts @@ -1,10 +1,9 @@ -import { assertSuccess, assertAnnotated, assertMultipleAnnotated } from './testHelper'; -import { Replacement } from 'tslint'; -import { expect } from 'chai'; -import { FsFileResolver } from '../src/angular/fileResolver/fsFileResolver'; -import { MetadataReader } from '../src/angular/metadataReader'; +import {assertSuccess, assertAnnotated, assertMultipleAnnotated} from './testHelper'; +import {Replacement} from 'tslint'; +import {expect} from 'chai'; +import {FsFileResolver} from '../src/angular/fileResolver/fsFileResolver'; +import {MetadataReader} from '../src/angular/metadataReader'; import * as ts from 'typescript'; -import { ComponentMetadata } from '../src/angular/metadata'; import chai = require('chai'); const getAst = (code: string, file = 'file.ts') => { @@ -109,13 +108,51 @@ describe('angular-whitespace', () => { assertSuccess('angular-whitespace', ast, ['check-pipe']); }); }); - }); - describe('failure', () => { + describe('check-semicolon', () => { + + it('should work with proper style', () => { + let source = ` + @Component({ + template: \`
\` + }) + class Bar {} + `; + assertSuccess('angular-whitespace', source, ['check-semicolon']); + }); - describe('interpolation', () => { - it('should fail when no space', () => { + it('should work with proper style also', () => { let source = ` + @Component({ + template: \`
\` + }) + class Bar {} + `; + assertSuccess('angular-whitespace', source, ['check-semicolon']); + }); + + it('should work with proper style also', () => { + let source = ` + @Component({ + template: \`
\` + }) + class Bar {} + `; + assertSuccess('angular-whitespace', source, ['check-semicolon']); + }); + + }); + + }); + + +}); + +describe('failure', () => { + + describe('interpolation', () => { + it('should fail when no space', () => { + let source = ` @Component({ template: \`
{{foo}}
@@ -124,16 +161,16 @@ describe('angular-whitespace', () => { }) class Bar {} `; - assertAnnotated({ - ruleName: 'angular-whitespace', - message: 'Missing whitespace in interpolation; expecting {{ expr }}', - source, - options: ['check-interpolation'] - }); + assertAnnotated({ + ruleName: 'angular-whitespace', + message: 'Missing whitespace in interpolation; expecting {{ expr }}', + source, + options: ['check-interpolation'] }); + }); - it('should fail when no space', () => { - let source = ` + it('should fail when no space', () => { + let source = ` @Component({ template: \`
{{foo }}
@@ -142,17 +179,17 @@ describe('angular-whitespace', () => { }) class Bar {} `; - assertAnnotated({ - ruleName: 'angular-whitespace', - message: 'Missing whitespace in interpolation; expecting {{ expr }}', - source, - options: ['check-interpolation'] - }); + assertAnnotated({ + ruleName: 'angular-whitespace', + message: 'Missing whitespace in interpolation; expecting {{ expr }}', + source, + options: ['check-interpolation'] }); + }); - describe('replacements', () => { - it('should fail and apply proper replacements when style is incorrect', () => { - let source = ` + describe('replacements', () => { + it('should fail and apply proper replacements when style is incorrect', () => { + let source = ` @Component({ template: \`
{{foo}}
@@ -160,15 +197,15 @@ describe('angular-whitespace', () => { \` }) class Bar {}`; - const failures = assertAnnotated({ - ruleName: 'angular-whitespace', - message: 'Missing whitespace in interpolation; expecting {{ expr }}', - source, - options: ['check-interpolation'] - }); - - const res = Replacement.applyAll(source, failures[0].getFix()); - expect(res).to.eq(` + const failures = assertAnnotated({ + ruleName: 'angular-whitespace', + message: 'Missing whitespace in interpolation; expecting {{ expr }}', + source, + options: ['check-interpolation'] + }); + + const res = Replacement.applyAll(source, failures[0].getFix()); + expect(res).to.eq(` @Component({ template: \`
{{ foo }}
@@ -176,10 +213,10 @@ describe('angular-whitespace', () => { \` }) class Bar {}`); - }); + }); - it('should fail and apply proper replacements when style is incorrect', () => { - let source = ` + it('should fail and apply proper replacements when style is incorrect', () => { + let source = ` @Component({ template: \`
{{foo }}
@@ -187,15 +224,15 @@ describe('angular-whitespace', () => { \` }) class Bar {}`; - const failures = assertAnnotated({ - ruleName: 'angular-whitespace', - message: 'Missing whitespace in interpolation; expecting {{ expr }}', - source, - options: ['check-interpolation'] - }); - - const res = Replacement.applyAll(source, failures[0].getFix()); - expect(res).to.eq(` + const failures = assertAnnotated({ + ruleName: 'angular-whitespace', + message: 'Missing whitespace in interpolation; expecting {{ expr }}', + source, + options: ['check-interpolation'] + }); + + const res = Replacement.applyAll(source, failures[0].getFix()); + expect(res).to.eq(` @Component({ template: \`
{{ foo }}
@@ -203,10 +240,10 @@ describe('angular-whitespace', () => { \` }) class Bar {}`); - }); + }); - it('should remove extra spaces', () => { - let source = ` + it('should remove extra spaces', () => { + let source = ` @Component({ template: \`
{{ foo }}
@@ -214,15 +251,15 @@ describe('angular-whitespace', () => { \` }) class Bar {}`; - const failures = assertAnnotated({ - ruleName: 'angular-whitespace', - message: 'Extra whitespace in interpolation; expecting {{ expr }}', - source, - options: ['check-interpolation'] - }); - - const res = Replacement.applyAll(source, failures[0].getFix()); - expect(res).to.eq(` + const failures = assertAnnotated({ + ruleName: 'angular-whitespace', + message: 'Extra whitespace in interpolation; expecting {{ expr }}', + source, + options: ['check-interpolation'] + }); + + const res = Replacement.applyAll(source, failures[0].getFix()); + expect(res).to.eq(` @Component({ template: \`
{{ foo }}
@@ -230,15 +267,126 @@ describe('angular-whitespace', () => { \` }) class Bar {}`); - }); }); }); - }); - describe('pipes', () => { - it('should fail when extra space after "|"', () => { + describe('check-semicolon', () => { + + it('should fail when no space after semicolon', () => { let source = ` + @Component({ + template: \`
+ ~~ + \` + }) + class Bar {} + `; + assertAnnotated({ + ruleName: 'angular-whitespace', + message: 'Missing whitespace after semicolon; expecting \'; expr\'', + source, + options: ['check-semicolon'] + }); + }); + + it('should fail when no space after semicolon', () => { + let source = ` + @Component({ + template: \`
+ ~~ + \` + }) + class Bar {} + `; + assertAnnotated({ + ruleName: 'angular-whitespace', + message: 'Missing whitespace after semicolon; expecting \'; expr\'', + source, + options: ['check-semicolon'] + }); + }); + + + it('should fail when no space after semicolon', () => { + let source = ` + @Component({ + template: \`
+ ~~ + \` + }) + class Bar {} + `; + assertAnnotated({ + ruleName: 'angular-whitespace', + message: 'Missing whitespace after semicolon; expecting \'; expr\'', + source, + options: ['check-semicolon'] + }); + }); + + describe('replacements', () => { + it('should fail when no space after semicolon', () => { + let source = ` + @Component({ + template: \`
+ ~~ + \` + }) + class Bar {} + `; + const failures = assertAnnotated({ + ruleName: 'angular-whitespace', + message: 'Missing whitespace after semicolon; expecting \'; expr\'', + source, + options: ['check-semicolon'] + }); + + const res = Replacement.applyAll(source, failures[0].getFix()); + expect(res).to.eq(` + @Component({ + template: \`
+ ~~ + \` + }) + class Bar {} + `); + }); + + it('should fail when no space after semicolon', () => { + let source = ` + @Component({ + template: \`
+ ~~ + \` + }) + class Bar {} + `; + const failures = assertAnnotated({ + ruleName: 'angular-whitespace', + message: 'Missing whitespace after semicolon; expecting \'; expr\'', + source, + options: ['check-semicolon'] + }); + + const res = Replacement.applyAll(source, failures[0].getFix()); + expect(res).to.eq(` + @Component({ + template: \`
+ ~~ + \` + }) + class Bar {} + `); + }); + + }); + }); +}); + +describe('pipes', () => { + it('should fail when extra space after "|"', () => { + let source = ` @Component({ selector: 'foo', template: \` @@ -250,15 +398,15 @@ describe('angular-whitespace', () => { foo: any; } `; - const failures = assertAnnotated({ - ruleName: 'angular-whitespace', - message: 'The pipe operator should be surrounded by one space on each side, i.e. " | ".', - source, - options: ['check-pipe'] - }); + const failures = assertAnnotated({ + ruleName: 'angular-whitespace', + message: 'The pipe operator should be surrounded by one space on each side, i.e. " | ".', + source, + options: ['check-pipe'] + }); - const res = Replacement.applyAll(source, failures[0].getFix()); - expect(res).to.eq(` + const res = Replacement.applyAll(source, failures[0].getFix()); + expect(res).to.eq(` @Component({ selector: 'foo', template: \` @@ -270,10 +418,10 @@ describe('angular-whitespace', () => { foo: any; } `); - }); + }); - it('should fail when extra space on both sides', () => { - let source = ` + it('should fail when extra space on both sides', () => { + let source = ` @Component({ selector: 'foo', template: \` @@ -285,15 +433,15 @@ describe('angular-whitespace', () => { foo: any; } `; - const failures = assertAnnotated({ - ruleName: 'angular-whitespace', - message: 'The pipe operator should be surrounded by one space on each side, i.e. " | ".', - source, - options: ['check-pipe'] - }); + const failures = assertAnnotated({ + ruleName: 'angular-whitespace', + message: 'The pipe operator should be surrounded by one space on each side, i.e. " | ".', + source, + options: ['check-pipe'] + }); - const res = Replacement.applyAll(source, failures[0].getFix()); - expect(res).to.eq(` + const res = Replacement.applyAll(source, failures[0].getFix()); + expect(res).to.eq(` @Component({ selector: 'foo', template: \` @@ -305,10 +453,10 @@ describe('angular-whitespace', () => { foo: any; } `); - }); + }); - it('should fail when extra space on both sides', () => { - let source = ` + it('should fail when extra space on both sides', () => { + let source = ` @Component({ selector: 'foo', template: \` @@ -320,15 +468,15 @@ describe('angular-whitespace', () => { foo: any; } `; - const failures = assertAnnotated({ - ruleName: 'angular-whitespace', - message: 'The pipe operator should be surrounded by one space on each side, i.e. " | ".', - source, - options: ['check-pipe'] - }); + const failures = assertAnnotated({ + ruleName: 'angular-whitespace', + message: 'The pipe operator should be surrounded by one space on each side, i.e. " | ".', + source, + options: ['check-pipe'] + }); - const res = Replacement.applyAll(source, failures[0].getFix()); - expect(res).to.eq(` + const res = Replacement.applyAll(source, failures[0].getFix()); + expect(res).to.eq(` @Component({ selector: 'foo', template: \` @@ -340,10 +488,10 @@ describe('angular-whitespace', () => { foo: any; } `); - }); + }); - it('should fail when no space', () => { - let source = ` + it('should fail when no space', () => { + let source = ` @Component({ selector: 'foo', template: \` @@ -355,15 +503,15 @@ describe('angular-whitespace', () => { foo: any; } `; - const failures = assertAnnotated({ - ruleName: 'angular-whitespace', - message: 'The pipe operator should be surrounded by one space on each side, i.e. " | ".', - source, - options: ['check-pipe'] - }); + const failures = assertAnnotated({ + ruleName: 'angular-whitespace', + message: 'The pipe operator should be surrounded by one space on each side, i.e. " | ".', + source, + options: ['check-pipe'] + }); - const res = Replacement.applyAll(source, failures[0].getFix()); - expect(res).to.eq(` + const res = Replacement.applyAll(source, failures[0].getFix()); + expect(res).to.eq(` @Component({ selector: 'foo', template: \` @@ -375,10 +523,10 @@ describe('angular-whitespace', () => { foo: any; } `); - }); + }); - it('should fail when no space', () => { - let source = ` + it('should fail when no space', () => { + let source = ` @Component({ selector: 'foo', template: \` @@ -390,18 +538,18 @@ describe('angular-whitespace', () => { foo: any; } `; - const failures = assertMultipleAnnotated({ - ruleName: 'angular-whitespace', - failures: [ - { char: '~', msg: 'The pipe operator should be surrounded by one space on each side, i.e. " | ".', }, - { char: '^', msg: 'The pipe operator should be surrounded by one space on each side, i.e. " | ".', }, - ], - source, - options: ['check-pipe'] - }); - const fixes = [].concat.apply([], failures.map(f => f.getFix())); - const res = Replacement.applyAll(source, fixes); - expect(res).to.eq(` + const failures = assertMultipleAnnotated({ + ruleName: 'angular-whitespace', + failures: [ + {char: '~', msg: 'The pipe operator should be surrounded by one space on each side, i.e. " | ".', }, + {char: '^', msg: 'The pipe operator should be surrounded by one space on each side, i.e. " | ".', }, + ], + source, + options: ['check-pipe'] + }); + const fixes = [].concat.apply([], failures.map(f => f.getFix())); + const res = Replacement.applyAll(source, fixes); + expect(res).to.eq(` @Component({ selector: 'foo', template: \` @@ -413,10 +561,10 @@ describe('angular-whitespace', () => { foo: any; } `); - }); + }); - it('should fail when no space in property binding', () => { - let source = ` + it('should fail when no space in property binding', () => { + let source = ` @Component({ selector: 'foo', template: \` @@ -428,15 +576,15 @@ describe('angular-whitespace', () => { foo: any; } `; - const failures = assertAnnotated({ - ruleName: 'angular-whitespace', - message: 'The pipe operator should be surrounded by one space on each side, i.e. " | ".', - source, - options: ['check-pipe'] - }); + const failures = assertAnnotated({ + ruleName: 'angular-whitespace', + message: 'The pipe operator should be surrounded by one space on each side, i.e. " | ".', + source, + options: ['check-pipe'] + }); - const res = Replacement.applyAll(source, failures[0].getFix()); - expect(res).to.eq(` + const res = Replacement.applyAll(source, failures[0].getFix()); + expect(res).to.eq(` @Component({ selector: 'foo', template: \` @@ -448,7 +596,6 @@ describe('angular-whitespace', () => { foo: any; } `); - }); }); - }); +