-
Notifications
You must be signed in to change notification settings - Fork 885
Add new rule: grouped-imports #3064
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,179 @@ | ||
/** | ||
* @license | ||
* Copyright 2017 Palantir Technologies, Inc. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
import * as Lint from "tslint"; | ||
import { isImportDeclaration, isTextualLiteral } from "tsutils"; | ||
import * as ts from "typescript"; | ||
|
||
export class Rule extends Lint.Rules.AbstractRule { | ||
/* tslint:disable:object-literal-sort-keys */ | ||
public static metadata: Lint.IRuleMetadata = { | ||
ruleName: "grouped-imports", | ||
description: "Separate import groups by blank lines.", | ||
rationale: "Keeps a clear overview on dependencies.", | ||
optionsDescription: "Not configurable.", | ||
hasFix: true, | ||
options: {}, | ||
optionExamples: [true], | ||
type: "style", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would actually argue that this falls into "maintainability" |
||
typescriptOnly: false, | ||
}; | ||
/* tslint:enable:object-literal-sort-keys */ | ||
|
||
public static GROUPED_IMPORTS = | ||
"Import sources of different groups must be sorted by: libraries, parent directories, current directory"; | ||
|
||
public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { | ||
return this.applyWithWalker(new Walker(sourceFile, this.ruleName, this.getOptions())); | ||
} | ||
} | ||
|
||
enum ImportStatementType { | ||
LIBRARY_IMPORT = 1, | ||
PARENT_DIRECTORY_IMPORT = 2, // starts with "../" | ||
CURRENT_DIRECTORY_IMPORT = 3, // starts with "./" | ||
} | ||
|
||
interface ImportStatement { | ||
statement: ts.ImportDeclaration; | ||
type: ImportStatementType; | ||
lineStart: number; | ||
lineEnd: number; | ||
} | ||
|
||
class Walker extends Lint.AbstractWalker<Lint.IOptions> { | ||
private previousImportStatement: ImportStatement | undefined; | ||
|
||
public walk(sourceFile: ts.SourceFile): void { | ||
const importsStatements = sourceFile.statements | ||
.filter(isImportDeclaration) | ||
.map(this.toImportStatement, this); | ||
const firstFailure = importsStatements.find(this.isBadlyPositioned, this); | ||
if (firstFailure != null) { | ||
const fix = this.createFix(importsStatements); | ||
this.addFailureAtNode(firstFailure.statement, Rule.GROUPED_IMPORTS, fix); | ||
} | ||
} | ||
|
||
private isBadlyPositioned(importStatement: ImportStatement): boolean { | ||
if (this.previousImportStatement != null) { | ||
if (importStatement.type === this.previousImportStatement.type) { | ||
if (importStatement.lineStart !== this.previousImportStatement.lineEnd + 1) { | ||
return true; | ||
} | ||
} else { | ||
if (importStatement.type < this.previousImportStatement.type) { | ||
return true; | ||
} else if (importStatement.lineStart !== this.previousImportStatement.lineEnd + 2) { | ||
return true; | ||
} | ||
} | ||
} | ||
this.previousImportStatement = importStatement; | ||
return false; | ||
} | ||
|
||
private toImportStatement(statement: ts.ImportDeclaration): ImportStatement { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: slightly better name would be |
||
return { | ||
lineEnd: this.sourceFile.getLineAndCharacterOfPosition(statement.getEnd()).line, | ||
lineStart: this.sourceFile.getLineAndCharacterOfPosition(statement.getStart()).line, | ||
statement, | ||
type: this.getImportStatementType(statement), | ||
}; | ||
} | ||
|
||
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; | ||
} | ||
} | ||
|
||
private getImportPath(statement: ts.ImportDeclaration): string { | ||
if (isTextualLiteral(statement.moduleSpecifier)) { | ||
return statement.moduleSpecifier.text; | ||
} | ||
return ""; | ||
} | ||
|
||
private createFix(importStatements: ImportStatement[]): Lint.Fix { | ||
const newLine = this.getEolChar(this.sourceFile); | ||
const imports = this.getOrderedImports(importStatements); | ||
const firstStatement = importStatements[0].statement; | ||
const addition = Lint.Replacement.appendText(firstStatement.getStart(this.sourceFile), imports.join(newLine)); | ||
const lastIndex = importStatements.length - 1; | ||
const deletions = importStatements.map((imp, index) => { | ||
const [start, end] = this.getRangeIncludingWhitespace(imp.statement, index === 0, index === lastIndex); | ||
return Lint.Replacement.deleteFromTo(start, end); | ||
}); | ||
return [...deletions, addition]; | ||
} | ||
|
||
private getOrderedImports(importStatements: ImportStatement[]): string[] { | ||
const libs = importStatements.filter((imp) => imp.type === ImportStatementType.LIBRARY_IMPORT); | ||
const parent = importStatements.filter((imp) => imp.type === ImportStatementType.PARENT_DIRECTORY_IMPORT); | ||
const current = importStatements.filter((imp) => imp.type === ImportStatementType.CURRENT_DIRECTORY_IMPORT); | ||
return [libs, parent, current].reduce((arr, imps) => { | ||
if (imps.length == 0) { | ||
return arr; | ||
} | ||
return arr.concat(imps.map((imp) => imp.statement.getText(this.sourceFile)), ""); | ||
}, [] as string[]).concat(""); | ||
} | ||
|
||
private getRangeIncludingWhitespace(statement: ts.ImportDeclaration, | ||
fixedStart: boolean, | ||
fixedEnd: boolean): [number, number] { | ||
const text = this.sourceFile.text; | ||
let start = statement.getStart(this.sourceFile); | ||
if (!fixedStart) { | ||
while (this.isWhiteSpaceChar(text[start - 1])) { | ||
start--; | ||
} | ||
} | ||
let end = statement.getEnd(); | ||
if (!fixedEnd) { | ||
while (this.isWhiteSpaceChar(text[end + 1])) { | ||
end++; | ||
} | ||
} | ||
return [start, end]; | ||
} | ||
|
||
private isWhiteSpaceChar(char: string | undefined): boolean { | ||
return char === undefined ? false : Lint.isWhiteSpace(char.charCodeAt(0)); | ||
} | ||
|
||
private getEolChar(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"; | ||
} | ||
} | ||
return newLine == null ? ts.sys.newLine : newLine; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
import {foo} from 'foo'; | ||
|
||
import {bar} from '../bar'; | ||
|
||
import './baz'; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
import {bar} from '../bar'; | ||
|
||
import {foo} from 'foo'; | ||
~~~~~~~~~~~~~~~~~~~~~~~~ [Import sources of different groups must be sorted by: libraries, parent directories, current directory] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please add a test that has both kinds of error: unnecessary blank line and wrong order There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For now, I only add a single failure & fix in case of wrong orders. I've added a test with both cases though. More on this in a later comment. |
||
|
||
import './baz'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
import {foo} from 'foo'; | ||
|
||
import {bar} from '../bar'; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
import {foo} from 'foo'; | ||
import {bar} from '../bar'; | ||
~~~~~~~~~~~~~~~~~~~~~~~~~~~ [Import sources of different groups must be sorted by: libraries, parent directories, current directory] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
import {foo} from 'foo'; | ||
|
||
import {bar} from '../bar'; | ||
|
||
import './baz'; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
import {foo} from 'foo'; | ||
import {bar} from 'bar'; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
import {foo} from 'foo'; | ||
|
||
import {bar} from 'bar'; | ||
~~~~~~~~~~~~~~~~~~~~~~~~ [Import sources of different groups must be sorted by: libraries, parent directories, current directory] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
import bar from "bar"; | ||
import foo from "foo"; | ||
|
||
import {baz} from "./baz"; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
import bar from "bar";import foo from "foo";import {baz} from "./baz"; | ||
~~~~~~~~~~~~~~~~~~~~~~ [Import sources of different groups must be sorted by: libraries, parent directories, current directory] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
#!/usr/bin/env node | ||
|
||
import {foo} from 'foo'; | ||
|
||
import {bar} from '../bar'; | ||
|
||
import './baz'; | ||
|
||
let bar2 = false; | ||
|
||
const one = 1; // some comment | ||
|
||
const two = 2; | ||
// another comment | ||
|
||
let q = 'a'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
#!/usr/bin/env node | ||
|
||
import {bar} from '../bar';let bar2 = false; | ||
|
||
const one = 1; | ||
|
||
import {foo} from 'foo'; // some comment | ||
~~~~~~~~~~~~~~~~~~~~~~~~ [Import sources of different groups must be sorted by: libraries, parent directories, current directory] | ||
|
||
const two = 2; | ||
// another comment | ||
|
||
import './baz'; | ||
|
||
let q = 'a'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
{ | ||
"rules": { | ||
"grouped-imports": true | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs better docs. it's unclear what import groups are from this simple description. I think the phrase "import statement" needs to appear. and the exact algorithm (libraries, parent directories, current directory) needs to be explained in
descriptionDetails
ofIRuleMetadata
.