Process some first review comments

* Use newline chars from sourceFile; * Add all imports fix as *only* fix; * Read path using TextualLiteral; * Rename & split checkForFailure; * Remove static methods; * Ignore rule in existing tests; * Add additional tests.
palantir · Jul 26, 2017 · f9b5d0b · f9b5d0b
1 parent a41644c
commit f9b5d0b
Show file tree

Hide file tree

Showing 7 changed files with 104 additions and 55 deletions.
diff --git a/src/rules/groupedImportsRule.ts b/src/rules/groupedImportsRule.ts
@@ -16,7 +16,7 @@
  */
 
 import * as Lint from "tslint";
-import { isImportDeclaration } from "tsutils";
+import { isImportDeclaration, isTextualLiteral } from "tsutils";
 import * as ts from "typescript";
 
 export class Rule extends Lint.Rules.AbstractRule {
@@ -47,8 +47,8 @@ export class Rule extends Lint.Rules.AbstractRule {
 
 enum ImportStatementType {
     LIBRARY_IMPORT = 1,
-    PARENT_DIRECTORY_IMPORT = 2,
-    CURRENT_DIRECTORY_IMPORT = 3,
+    PARENT_DIRECTORY_IMPORT = 2, // starts with "../"
+    CURRENT_DIRECTORY_IMPORT = 3, // starts with "./"
 }
 
 interface ImportStatement {
@@ -60,93 +60,123 @@ interface ImportStatement {
 
 class Walker extends Lint.AbstractWalker<Lint.IOptions> {
     private lastImportStatement: ImportStatement;
+    private newLine: string;
+    private allImportsFix: boolean;
 
-    private static getImportStatementType(statement: ts.Statement): ImportStatementType {
-        const path = Walker.getImportPath(statement);
-        if (path.charAt(0) === ".") {
-            if (path.charAt(1) === ".") {
-                return ImportStatementType.PARENT_DIRECTORY_IMPORT;
-            } else {
-                return ImportStatementType.CURRENT_DIRECTORY_IMPORT;
+    public walk(sourceFile: ts.SourceFile): void {
+        this.newLine = this.getEofChar(sourceFile);
+        sourceFile.statements
+            .filter(isImportDeclaration)
+            .forEach(this.checkStatement);
+    }
+
+    private getEofChar(sourceFile: ts.SourceFile): string {
+        const lineEnd = sourceFile.getLineEndOfPosition(0);
+        let newLine;
+        if (lineEnd > 0) {
+            if (lineEnd > 1 && sourceFile.text[lineEnd - 1] === "\r") {
+                newLine = "\r\n";
+            } else if (sourceFile.text[lineEnd] === "\n") {
+                newLine = "\n";
             }
-        } else {
-            return ImportStatementType.LIBRARY_IMPORT;
         }
+        return newLine == null ? ts.sys.newLine : newLine;
     }
 
-    private static getImportPath(statement: ts.Statement): string {
-        const str = statement.getText();
-        let index;
-        let lastIndex;
-        index = str.indexOf("'");
-        if (index > 0) {
-            lastIndex = str.lastIndexOf("'");
-        } else {
-            index = str.indexOf("\"");
-            lastIndex = str.lastIndexOf("\"");
+    private checkStatement = (statement: ts.ImportDeclaration): void => {
+        if (this.allImportsFix) {
+            return;
         }
-        if (index < 0 || lastIndex < 0) {
-            throw new Error(`Unable to extract path from import statement \`${statement.getText()}\``);
+        const importStatement = this.toImportStatement(statement);
+        if (this.lastImportStatement != null) {
+            this.checkForFailure(importStatement);
         }
-        return str.substring(index + 1, lastIndex);
-    }
-
-    public walk(sourceFile: ts.SourceFile): void {
-        sourceFile.statements
-            .filter(isImportDeclaration)
-            .forEach((st) => this.checkStatement(st));
+        this.lastImportStatement = importStatement;
     }
 
-    private toImportStatement(statement: ts.Statement): ImportStatement {
+    private toImportStatement(statement: ts.ImportDeclaration): ImportStatement {
         return {
             lineEnd: this.sourceFile.getLineAndCharacterOfPosition(statement.getEnd()).line,
             lineStart: this.sourceFile.getLineAndCharacterOfPosition(statement.getStart()).line,
             statement,
-            type: Walker.getImportStatementType(statement),
+            type: this.getImportStatementType(statement),
         };
     }
 
-    private checkStatement(statement: ts.Statement): void {
-        const importStatement = this.toImportStatement(statement);
-        if (this.lastImportStatement) {
-            this.checkImportStatement(importStatement);
+    private getImportStatementType(statement: ts.ImportDeclaration): ImportStatementType {
+        const path = this.getImportPath(statement);
+        if (path.charAt(0) === ".") {
+            if (path.charAt(1) === ".") {
+                return ImportStatementType.PARENT_DIRECTORY_IMPORT;
+            } else {
+                return ImportStatementType.CURRENT_DIRECTORY_IMPORT;
+            }
+        } else {
+            return ImportStatementType.LIBRARY_IMPORT;
         }
-        this.lastImportStatement = importStatement;
     }
 
-    private checkImportStatement(importStatement: ImportStatement) {
+    private getImportPath(statement: ts.ImportDeclaration): string {
+        if (isTextualLiteral(statement.moduleSpecifier)) {
+            return statement.moduleSpecifier.text;
+        }
+        return "";
+    }
+
+    private checkForFailure(importStatement: ImportStatement): void {
         if (importStatement.type === this.lastImportStatement.type) {
             if (importStatement.lineStart !== this.lastImportStatement.lineEnd + 1) {
-                const replacement = Lint.Replacement.deleteFromTo(
-                    this.lastImportStatement.statement.getEnd() + 1, importStatement.statement.getStart());
-                this.addFailureAtNode(importStatement.statement, Rule.IMPORT_SOURCES_SEPARATED, replacement);
+                this.addSeparatedImportsFailure(importStatement);
             }
-        } else if (importStatement.type.valueOf() < this.lastImportStatement.type.valueOf()) {
-            this.addFailureAtNode(importStatement.statement, Rule.IMPORT_SOURCES_ORDER, this.getAllImportsFix());
         } else {
-            if (importStatement.lineStart !== this.lastImportStatement.lineEnd + 2) {
-                const replacement = Lint.Replacement.appendText(importStatement.statement.getStart(), ts.sys.newLine);
-                this.addFailureAtNode(importStatement.statement, Rule.IMPORT_SOURCES_NOT_SEPARATED, replacement);
+            if (importStatement.type < this.lastImportStatement.type) {
+                this.addIncorrectlyOrderedImportsFailure(importStatement);
+            } else if (importStatement.lineStart !== this.lastImportStatement.lineEnd + 2) {
+                this.addNotSeparatedImportsFailure(importStatement);
             }
         }
     }
 
+    private addSeparatedImportsFailure(importStatement: ImportStatement): void {
+        const text = [this.lastImportStatement, importStatement]
+            .map((st) => st.statement.getText())
+            .join(this.newLine);
+        const replacement = Lint.Replacement.replaceFromTo(
+            this.lastImportStatement.statement.getStart(), importStatement.statement.getEnd(), text);
+        this.addFailureAtNode(importStatement.statement, Rule.IMPORT_SOURCES_SEPARATED, replacement);
+    }
+
+    private addIncorrectlyOrderedImportsFailure(importStatement: ImportStatement): void {
+        this.allImportsFix = true;
+        this.failures.length = 0;
+        this.addFailureAtNode(importStatement.statement, Rule.IMPORT_SOURCES_ORDER, this.getAllImportsFix());
+    }
+
+    private addNotSeparatedImportsFailure(importStatement: ImportStatement): void {
+        const replacement = Lint.Replacement.replaceFromTo(this.lastImportStatement.statement.getEnd(),
+            importStatement.statement.getStart(), this.newLine + this.newLine);
+        this.addFailureAtNode(importStatement.statement, Rule.IMPORT_SOURCES_NOT_SEPARATED, replacement);
+    }
+
     private getAllImportsFix(): Lint.Fix {
         const importStatements = this.sourceFile.statements.filter(isImportDeclaration);
-        const libs = importStatements.filter((st) => Walker.getImportStatementType(st) === ImportStatementType.LIBRARY_IMPORT);
-        const parent = importStatements.filter((st) => Walker.getImportStatementType(st) === ImportStatementType.PARENT_DIRECTORY_IMPORT);
-        const current = importStatements.filter((st) => Walker.getImportStatementType(st) === ImportStatementType.CURRENT_DIRECTORY_IMPORT);
+        const libs = importStatements.filter(
+            (st) => this.getImportStatementType(st) === ImportStatementType.LIBRARY_IMPORT);
+        const parent = importStatements.filter(
+            (st) => this.getImportStatementType(st) === ImportStatementType.PARENT_DIRECTORY_IMPORT);
+        const current = importStatements.filter(
+            (st) => this.getImportStatementType(st) === ImportStatementType.CURRENT_DIRECTORY_IMPORT);
         let imports: string[] = [];
         [libs, parent, current].forEach((statements) => {
-            if (statements.length) {
+            if (statements.length > 0) {
                 imports = imports.concat(statements.map((st) => st.getText()));
                 imports.push("");
             }
         });
         return Lint.Replacement.replaceFromTo(
             importStatements[0].getStart(),
             importStatements[importStatements.length - 1].getEnd(),
-            imports.join(ts.sys.newLine),
+            imports.join(this.newLine),
         );
     }
 }
diff --git a/test/rules/grouped-imports/mixed.ts.fix b/test/rules/grouped-imports/mixed.ts.fix
@@ -0,0 +1,6 @@
+import {foo} from 'foo';
+
+import {bar} from '../bar';
+
+import './baz';
+
diff --git a/test/rules/grouped-imports/mixed.ts.lint b/test/rules/grouped-imports/mixed.ts.lint
@@ -0,0 +1,5 @@
+import {bar} from '../bar';
+
+import {foo} from 'foo';
+~~~~~~~~~~~~~~~~~~~~~~~~ [Import sources of different groups must be sorted by: libraries, parent directories, current directory]
+import './baz';
diff --git a/test/rules/grouped-imports/single-line.ts.fix b/test/rules/grouped-imports/single-line.ts.fix
@@ -0,0 +1,4 @@
+import bar from "bar";
+import foo from "foo";
+
+import {baz} from "./baz";
diff --git a/test/rules/grouped-imports/single-line.ts.lint b/test/rules/grouped-imports/single-line.ts.lint
@@ -0,0 +1,3 @@
+import bar from "bar";import foo from "foo";import {baz} from "./baz";
+                      ~~~~~~~~~~~~~~~~~~~~~~                           [Import sources within a group must not be separated by blank lines]
+                                            ~~~~~~~~~~~~~~~~~~~~~~~~~~ [Import sources of different groups must be separated by a single blank line]
diff --git a/tslint.json b/tslint.json
@@ -32,6 +32,7 @@
 
     // TODO: Enable these
     "completed-docs": false,
+    "grouped-imports": false,
     "no-any": false,
     "no-magic-numbers": false,
     "no-non-null-assertion": false,

diff --git a/yarn.lock b/yarn.lock
@@ -1502,9 +1502,9 @@ type-detect@^1.0.0:
   version "1.0.0"
   resolved "https://registry.yarnpkg.com/type-detect/-/type-detect-1.0.0.tgz#762217cc06db258ec48908a1298e8b95121e8ea2"
 
-typescript@^2.4.1:
-  version "2.4.1"
-  resolved "https://registry.yarnpkg.com/typescript/-/typescript-2.4.1.tgz#c3ccb16ddaa0b2314de031e7e6fee89e5ba346bc"
+typescript@~2.4.1:
+  version "2.4.2"
+  resolved "https://registry.yarnpkg.com/typescript/-/typescript-2.4.2.tgz#f8395f85d459276067c988aa41837a8f82870844"
 
 uglify-js@^2.6:
   version "2.8.28"