Skip to content

Commit

Permalink
Update orderedImports for standalone groups and source-order any
Browse files Browse the repository at this point in the history
This change updates the refactored "group" based orderedImports
code to handle the cases fixed in the following previous PRs:

- palantir#4374: fixed grouping for import-sources-order
- palantir#3733: check all imports of same type are grouped

As part of this we removed the concept of ImportType as this
is now covered by the defined groups for sorting.

Moved the check for ensuring groups are not standing alone
into the code checkBlocksGroup() code where other group checks
are done.

Ex:
```
import './baa';
import './baz';

import './caa';  // this should fail
```

Also had to update several other test cases to include the failure
notice about import groups needing to be together.
  • Loading branch information
abierbaum committed Jan 16, 2019
1 parent e68c575 commit d293ec8
Show file tree
Hide file tree
Showing 12 changed files with 75 additions and 39 deletions.
51 changes: 21 additions & 30 deletions src/rules/orderedImportsRule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -167,8 +167,8 @@ export class Rule extends Lint.Rules.AbstractRule {
"Import sources of different groups must be sorted by:";
public static IMPORT_SOURCES_UNORDERED = "Import sources within a group must be alphabetized.";
public static NAMED_IMPORTS_UNORDERED = "Named imports must be alphabetized.";
public static IMPORT_SOURCES_OF_SAME_TYPE_NOT_IN_ONE_GROUP =
"Import sources of the same type (package, same folder, different folder) must be grouped together.";
public static IMPORT_GROUPS_MUST_BE_TOGETHER =
"Import sources with same group order must be grouped together.";

public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] {
return this.applyWithWalker(
Expand Down Expand Up @@ -202,12 +202,6 @@ const TRANSFORMS = new Map<string, Transform>([
],
]);

enum ImportType {
LIBRARY_IMPORT = 1,
PARENT_DIRECTORY_IMPORT = 2, // starts with "../"
CURRENT_DIRECTORY_IMPORT = 3, // starts with "./"
}

interface Options {
/** Transform to use when sorting import source names. */
importSourcesOrderTransform: Transform;
Expand Down Expand Up @@ -405,14 +399,12 @@ class Walker extends Lint.AbstractWalker<Options> {
);

const prevImportPath = this.currentImportsBlock.getLastImportSource();
const type = getImportType(fullImportPath);

this.currentImportsBlock.addImportDeclaration(
this.sourceFile,
node,
importPath,
matchingGroup,
type,
);

if (prevImportPath !== null && compare(importPath, prevImportPath) === -1) {
Expand Down Expand Up @@ -476,13 +468,27 @@ class Walker extends Lint.AbstractWalker<Options> {
this.addFailureAtNode(decl.node, msg, this.getGroupOrderReplacements());
};

const blocksWithContent = this.importsBlocks.filter(
b => b.getImportDeclarations().length > 0,
);

// Check for groups that are not all together
// - note: replacements for this are handled by the later checks below
const groupOrdersSeen = new Set<number>();

for (const block of blocksWithContent) {
const firstImport = block.getImportDeclarations()[0];
const blockOrder = firstImport.group.order;
if (groupOrdersSeen.has(blockOrder)) {
this.addFailureAtNode(firstImport.node, Rule.IMPORT_GROUPS_MUST_BE_TOGETHER);
}
groupOrdersSeen.add(blockOrder);
}

// Check if each import group block is in order
// If not, then return failure with replacement for all groups in the file
for (const block of this.importsBlocks) {
for (const block of blocksWithContent) {
const importDeclarations = block.getImportDeclarations();
if (importDeclarations.length === 0) {
continue;
}

// check if group is out of order
const blockOrder = importDeclarations[0].group.order;
Expand All @@ -492,6 +498,7 @@ class Walker extends Lint.AbstractWalker<Options> {
}

// check if all declarations have the same order value
// and mark the first one that is out of order
for (const decl of importDeclarations) {
if (decl.group.order !== blockOrder) {
addFailingImportDecl(decl);
Expand Down Expand Up @@ -605,8 +612,6 @@ interface ImportDeclaration {
importPath: string;
/** details for the group that we match */
group: GroupOption;
// XXX: May be unused
type: ImportType;
}

/**
Expand All @@ -624,7 +629,6 @@ class ImportsBlock {
node: ImportDeclaration["node"],
importPath: string,
group: GroupOption,
type: ImportType,
) {
const start = this.getStartOffset(node);
const end = this.getEndOffset(sourceFile, node);
Expand All @@ -643,7 +647,6 @@ class ImportsBlock {
nodeEndOffset: end,
nodeStartOffset: start,
text,
type,
});
}

Expand Down Expand Up @@ -714,18 +717,6 @@ class ImportsBlock {
}
}

function getImportType(importPath: string): ImportType {
if (importPath.charAt(0) === ".") {
if (importPath.charAt(1) === ".") {
return ImportType.PARENT_DIRECTORY_IMPORT;
} else {
return ImportType.CURRENT_DIRECTORY_IMPORT;
}
} else {
return ImportType.LIBRARY_IMPORT;
}
}

// Convert aBcD --> AbCd
function flipCase(str: string): string {
return Array.from(str)
Expand Down
4 changes: 2 additions & 2 deletions test/rules/ordered-imports/grouped-imports/test.ts.lint
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,9 @@ import './baa';
~~~~~~~~~~~~~~~ [Import sources within a group must be alphabetized.]

import './caa';
~~~~~~~~~~~~~~~ [Import sources of the same type (package, same folder, different folder) must be grouped together.]
~~~~~~~~~~~~~~~ [Import sources with same group order must be grouped together.]

import x = require('y');
~~~~~~~~~~~~~~~~~~~~~~~~ [Import sources of the same type (package, same folder, different folder) must be grouped together.]
~~~~~~~~~~~~~~~~~~~~~~~~ [Import sources with same group order must be grouped together.]

export class Test {}
7 changes: 6 additions & 1 deletion test/rules/ordered-imports/grouped-imports/tslint.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
{
"rules": {
"ordered-imports": [true, {"import-sources-order": "case-insensitive", "named-imports-order": "case-insensitive", "grouped-imports": true}]
"ordered-imports": [
true, {
"import-sources-order": "case-insensitive",
"named-imports-order": "case-insensitive",
"grouped-imports": true
}]
}
}
3 changes: 3 additions & 0 deletions test/rules/ordered-imports/groups-complex/test.ts.lint
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,21 @@ import {x} from '@pkg/foo';
~~~~~~~~~~~~~~~~~~~~~~~~~~~ [Import sources within a group must be alphabetized.]

import {app_b} from 'app/bar';
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [Import sources with same group order must be grouped together.]
// comment pkg/bar
import {a} from '@pkg/bar';
~~~~~~~~~~~~~~~~~~~~~~~~~~~ [Import sources within a group must be alphabetized.]

import {xbar} from '../xbar';

import {bar} from '../bar';
~~~~~~~~~~~~~~~~~~~~~~~~~~~ [Import sources with same group order must be grouped together.]

import {foo, afoo} from 'foo';
~~~~~~~~~ [Named imports must be alphabetized.]

import x = require('y');
~~~~~~~~~~~~~~~~~~~~~~~~ [Import sources with same group order must be grouped together.]

import './baz'; // required
import './baa';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {a} from '@pkg/foo';

import {b} from '@pkg/bar';
~~~~~~~~~~~~~~~~~~~~~~~~~~~ [Import sources of different groups must be sorted by: ^app_b, ^app_a, ^@pkg .]
~~~~~~~~~~~~~~~~~~~~~~~~~~~ [Import sources with same group order must be grouped together.]

import {o} from 'other/bar';

Expand Down
2 changes: 2 additions & 0 deletions test/rules/ordered-imports/groups-string-list/test.ts.lint
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,13 @@ import { app_f } from "app/foo";
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [Import sources of different groups must be sorted by: ^app, ^@pkg, ^\.\., ^\. .]

import { a } from "@pkg/bar";
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [Import sources with same group order must be grouped together.]
import { app_b } from "app/bar";

import { xbar } from "../xbar";

import { bar } from "../bar";
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [Import sources with same group order must be grouped together.]

import { foo } from "foo";

Expand Down
3 changes: 3 additions & 0 deletions test/rules/ordered-imports/groups-unmatched/test.ts.lint
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,15 @@ import {x} from '@pkg/foo';

import {app_b} from 'app/bar';
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [Import sources of different groups must be sorted by: ^app, ^@pkg .]
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [Import sources with same group order must be grouped together.]

import {a} from '@pkg/bar';
~~~~~~~~~~~~~~~~~~~~~~~~~~~ [Import sources with same group order must be grouped together.]

import {xbar} from '../xbar';

import {foo} from 'foo';
~~~~~~~~~~~~~~~~~~~~~~~~ [Import sources with same group order must be grouped together.]
import {b} from './ybar';

export class Test {}
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,3 @@ import { getOr } from 'lodash/fp'
import { Request } from 'express'

import { getStatusCode } from './errors'

Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
import { getOr } from 'lodash/fp'

import { Request } from 'express'
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [1]
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [Import sources of different groups must be sorted by: libraries, parent directories, current directory .]
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [Import sources with same group order must be grouped together.]
import { getStatusCode } from './errors'
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [grouped-imports]

[grouped-imports]: Import sources of different groups must be sorted by: libraries, parent directories, current directory.
[1]: Import sources of the same type (package, same folder, different folder) must be grouped together.
10 changes: 10 additions & 0 deletions test/rules/ordered-imports/standalone-grouped-import/test.ts.fix
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
#!/usr/bin/env node

import {a} from 'foo';
import x = require('y');

import './baa';
import './baz';
import './caa';

export class Test {}
15 changes: 15 additions & 0 deletions test/rules/ordered-imports/standalone-grouped-import/test.ts.lint
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
#!/usr/bin/env node

import {a} from 'foo';

import './baa';
import './baz';

import './caa';
~~~~~~~~~~~~~~~ [Import sources of different groups must be sorted by: libraries, parent directories, current directory .]
~~~~~~~~~~~~~~~ [Import sources with same group order must be grouped together.]

import x = require('y');
~~~~~~~~~~~~~~~~~~~~~~~~ [Import sources with same group order must be grouped together.]

export class Test {}
10 changes: 10 additions & 0 deletions test/rules/ordered-imports/standalone-grouped-import/tslint.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"rules": {
"ordered-imports": [
true, {
"import-sources-order": "case-insensitive",
"named-imports-order": "case-insensitive",
"grouped-imports": true
}]
}
}

0 comments on commit d293ec8

Please sign in to comment.