feat(import-destructuring-spacing): add fixer (#595)

src/importDestructuringSpacingRule.ts

@@ -1,52 +1,73 @@
-import * as ts from 'typescript';
-import * as Lint from 'tslint';
+import { Fix, IOptions, IRuleMetadata, Replacement, RuleFailure, Rules, RuleWalker } from 'tslint/lib';
+import { NamedImports, SourceFile } from 'typescript/lib/typescript';
 
-export class Rule extends Lint.Rules.AbstractRule {
-  public static metadata: Lint.IRuleMetadata = {
-    ruleName: 'import-destructuring-spacing',
-    type: 'style',
+export class Rule extends Rules.AbstractRule {
+  static readonly metadata: IRuleMetadata = {
     description: 'Ensure consistent and tidy imports.',
-    rationale: "Imports are easier for the reader to look at when they're tidy.",
+    hasFix: true,
     options: null,
     optionsDescription: 'Not configurable.',
+    rationale: "Imports are easier for the reader to look at when they're tidy.",
+    ruleName: 'import-destructuring-spacing',
+    type: 'style',
     typescriptOnly: true
   };
 
-  public static FAILURE_STRING = "You need to leave whitespaces inside of the import statement's curly braces";
+  static readonly FAILURE_STRING = "Import statement's curly braces must be spaced exactly by a space to the right and a space to the left";
 
-  public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] {
+  apply(sourceFile: SourceFile): RuleFailure[] {
     return this.applyWithWalker(new ImportDestructuringSpacingWalker(sourceFile, this.getOptions()));
   }
 }
 
-// The walker takes care of all the work.
-class ImportDestructuringSpacingWalker extends Lint.RuleWalker {
-  private scanner: ts.Scanner;
+export const getFailureMessage = (): string => {
+  return Rule.FAILURE_STRING;
+};
+
+const isBlankOrMultilineImport = (value: string): boolean => {
+  return value.indexOf('\n') !== -1 || /^\{\s*\}$/.test(value);
+};
 
-  constructor(sourceFile: ts.SourceFile, options: Lint.IOptions) {
+class ImportDestructuringSpacingWalker extends RuleWalker {
+  constructor(sourceFile: SourceFile, options: IOptions) {
     super(sourceFile, options);
-    this.scanner = ts.createScanner(ts.ScriptTarget.ES5, false, ts.LanguageVariant.Standard, sourceFile.text);
   }
 
-  public visitImportDeclaration(node: ts.ImportDeclaration) {
-    const importClause = node.importClause;
-    if (importClause && importClause.namedBindings) {
-      const text = importClause.namedBindings.getText();
+  protected visitNamedImports(node: NamedImports): void {
+    this.validateNamedImports(node);
+    super.visitNamedImports(node);
+  }
+
+  private validateNamedImports(node: NamedImports): void {
+    const nodeText = node.getText();
 
-      if (!this.checkForWhiteSpace(text)) {
-        this.addFailure(
-          this.createFailure(importClause.namedBindings.getStart(), importClause.namedBindings.getWidth(), Rule.FAILURE_STRING)
-        );
-      }
+    if (isBlankOrMultilineImport(nodeText)) {
+      return;
+    }
+
+    const totalLeadingSpaces = nodeText.match(/^\{(\s*)/)[1].length;
+    const totalTrailingSpaces = nodeText.match(/(\s*)}$/)[1].length;
+
+    if (totalLeadingSpaces === 1 && totalTrailingSpaces === 1) {
+      return;
     }
-    // call the base version of this visitor to actually parse this node
-    super.visitImportDeclaration(node);
-  }
 
-  private checkForWhiteSpace(text: string) {
-    if (/\s*\*\s+as\s+[^\s]/.test(text)) {
-      return true;
+    const nodeStartPos = node.getStart();
+    const nodeEndPos = node.getEnd();
+    let fix: Fix = [];
+
+    if (totalLeadingSpaces === 0) {
+      fix.push(Replacement.appendText(nodeStartPos + 1, ' '));
+    } else if (totalLeadingSpaces > 1) {
+      fix.push(Replacement.deleteText(nodeStartPos + 1, totalLeadingSpaces - 1));
+    }
+
+    if (totalTrailingSpaces === 0) {
+      fix.push(Replacement.appendText(nodeEndPos - 1, ' '));
+    } else if (totalTrailingSpaces > 1) {
+      fix.push(Replacement.deleteText(nodeEndPos - totalTrailingSpaces, totalTrailingSpaces - 1));
     }
-    return /{\s[^]*\s}/.test(text);
+
+    this.addFailureAtNode(node, getFailureMessage(), fix);
   }
 }

test/importDestructuringSpacingRule.spec.ts

@@ -1,77 +1,196 @@
+import { expect } from 'chai';
+import { Replacement } from 'tslint/lib';
+import { getFailureMessage, Rule } from '../src/importDestructuringSpacingRule';
 import { assertAnnotated, assertSuccess } from './testHelper';
 
-describe('import-destructuring-spacing', () => {
-  describe('invalid import spacing', () => {
-    it('should fail when the imports have no spaces', () => {
-      let source = `
+const {
+  metadata: { ruleName }
+} = Rule;
+
+describe(ruleName, () => {
+  describe('failure', () => {
+    it('should fail when a single import has no spaces', () => {
+      const source = `
         import {Foo} from './foo'
                ~~~~~
       `;
       assertAnnotated({
-        ruleName: 'import-destructuring-spacing',
-        message: "You need to leave whitespaces inside of the import statement's curly braces",
+        message: getFailureMessage(),
+        ruleName,
         source
       });
     });
 
-    it('should fail with multiple items to import', () => {
-      let source = `
+    it('should fail when multiple imports have no spaces', () => {
+      const source = `
       import {Foo,Bar} from './foo'
              ~~~~~~~~~
       `;
-      assertAnnotated({
-        ruleName: 'import-destructuring-spacing',
-        message: "You need to leave whitespaces inside of the import statement's curly braces",
-        source
-      });
+      assertAnnotated({ message: getFailureMessage(), ruleName, source });
     });
 
-    it('should fail with spaces between items', () => {
-      let source = `
-        import {Foo,  Bar} from './foo'
-               ~~~~~~~~~~~
-      `;
-      assertAnnotated({
-        ruleName: 'import-destructuring-spacing',
-        message: "You need to leave whitespaces inside of the import statement's curly braces",
-        source
-      });
-    });
-
-    it('should fail with only one whitespace in the left', () => {
-      let source = `
+    it('should fail when there are no trailing spaces', () => {
+      const source = `
         import { Foo} from './foo';
                ~~~~~~
       `;
       assertAnnotated({
-        ruleName: 'import-destructuring-spacing',
-        message: "You need to leave whitespaces inside of the import statement's curly braces",
+        message: getFailureMessage(),
+        ruleName,
         source
       });
     });
 
-    it('should fail with only one whitespace in the right', () => {
-      let source = `
+    it('should fail when there are no leading spaces', () => {
+      const source = `
         import {Foo } from './foo';
                ~~~~~~
       `;
       assertAnnotated({
-        ruleName: 'import-destructuring-spacing',
-        message: "You need to leave whitespaces inside of the import statement's curly braces",
+        message: getFailureMessage(),
+        ruleName,
         source
       });
     });
   });
 
-  describe('valid import spacing', () => {
+  describe('failure with replacements', () => {
+    it('should fail and apply proper replacements when there are no spaces', () => {
+      const source = `
+        import {Foo} from './foo';
+               ~~~~~
+      `;
+      const failures = assertAnnotated({ message: getFailureMessage(), ruleName, source });
+
+      if (!Array.isArray(failures)) {
+        return;
+      }
+
+      const replacement = Replacement.applyFixes(source, failures.map(f => f.getFix()));
+
+      expect(replacement).to.eq(`
+        import { Foo } from './foo';
+               ~~~~~
+      `);
+    });
+
+    it('should fail and apply proper replacements when there is more than one leading space', () => {
+      const source = `
+        import {     Bar, BarFoo, Foo } from './foo';
+               ~~~~~~~~~~~~~~~~~~~~~~~~
+      `;
+      const failures = assertAnnotated({ message: getFailureMessage(), ruleName, source });
+
+      if (!Array.isArray(failures)) {
+        return;
+      }
+
+      const replacement = Replacement.applyFixes(source, failures.map(f => f.getFix()));
+
+      expect(replacement).to.eq(`
+        import { Bar, BarFoo, Foo } from './foo';
+               ~~~~~~~~~~~~~~~~~~~~~~~~
+      `);
+    });
+
+    it('should fail and apply proper replacements when there is more than one trailing space', () => {
+      const source = `
+        import { Bar, BarFoo, Foo     } from './foo';
+               ~~~~~~~~~~~~~~~~~~~~~~~~
+      `;
+      const failures = assertAnnotated({ message: getFailureMessage(), ruleName, source });
+
+      if (!Array.isArray(failures)) {
+        return;
+      }
+
+      const replacement = Replacement.applyFixes(source, failures.map(f => f.getFix()));
+
+      expect(replacement).to.eq(`
+        import { Bar, BarFoo, Foo } from './foo';
+               ~~~~~~~~~~~~~~~~~~~~~~~~
+      `);
+    });
+
+    it('should fail and apply proper replacements when there is more than one space left and right', () => {
+      const source = `
+        import {     Bar, BarFoo, Foo     } from './foo';
+               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+      `;
+      const failures = assertAnnotated({ message: getFailureMessage(), ruleName, source });
+
+      if (!Array.isArray(failures)) {
+        return;
+      }
+
+      const replacement = Replacement.applyFixes(source, failures.map(f => f.getFix()));
+
+      expect(replacement).to.eq(`
+        import { Bar, BarFoo, Foo } from './foo';
+               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+      `);
+    });
+  });
+
+  describe('success', () => {
     it('should succeed with valid spacing', () => {
-      let source = "import { Foo } from './foo';";
-      assertSuccess('import-destructuring-spacing', source);
+      const source = `
+        import { Foo } from './foo';
+      `;
+      assertSuccess(ruleName, source);
+    });
+
+    it('should succeed with multiple spaces between imports', () => {
+      const source = `
+        import { Bar,    Foo } from './foo';
+      `;
+      assertSuccess(ruleName, source);
+    });
+
+    it('should succeed for blank imports', () => {
+      const source = `
+        import {} from 'foo';
+      `;
+      assertSuccess(ruleName, source);
+    });
+
+    it('should succeed for module imports', () => {
+      const source = `
+        import foo = require('./foo');
+      `;
+      assertSuccess(ruleName, source);
+    });
+
+    it('should succeed for patch imports', () => {
+      const source = `
+        import 'rxjs/add/operator/map';
+      `;
+      assertSuccess(ruleName, source);
     });
 
-    it('should work with alias imports', () => {
-      let source = "import * as Foo from './foo';";
-      assertSuccess('import-destructuring-spacing', source);
+    it('should succeed with alias imports', () => {
+      const source = `
+        import * as Foo from './foo';
+      `;
+      assertSuccess(ruleName, source);
+    });
+
+    it('should succeed for alias imports inside braces', () => {
+      const source = `
+        import { default as _rollupMoment, Moment } from 'moment';
+      `;
+      assertSuccess(ruleName, source);
+    });
+
+    it('should succeed for multiline imports', () => {
+      const source = `
+        import {
+          Bar,
+          BarFoo,
+          Foo
+        } from './foo';
+      `;
+      assertSuccess(ruleName, source);
     });
   });
 });