Add new rule: grouped-imports #3064

eriktim · 2017-07-21T14:52:29Z

PR checklist

Addresses an existing issue: #0000
New feature, bugfix, or enhancement
- Includes tests
Documentation update

Overview of change:

Adds new rule to verify imports are grouped by libraries, parent directories & current directory.

Is there anything you'd like reviewers to focus on?

The fixes for too many/few blank lines fix a single import statement. The fix for changing the order fixes all imports statements completely. I am not sure this is OK.

CHANGELOG.md entry:

[new-rule] grouped-imports

palantirtech · 2017-07-21T14:52:30Z

Thanks for your interest in palantir/tslint, @trenneman! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

ajafff

Finished my first review.
The fixer of this rule should not break any code.

Another related question is how comments between imports should be handled? Treated as blank lines, removed while fixing, etc.?

Also CI is failing probably because the existing code in this repo does not conform to this new rule. It would be the best to just disable it in tslint.json

ajafff · 2017-07-24T19:10:08Z

test/rules/grouped-imports/bad-order.ts.lint

+import {bar} from '../bar';
+
+import {foo} from 'foo';
+~~~~~~~~~~~~~~~~~~~~~~~~ [Import sources of different groups must be sorted by: libraries, parent directories, current directory]


please add a test that has both kinds of error: unnecessary blank line and wrong order

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.

ajafff · 2017-07-24T19:13:57Z

src/rules/groupedImportsRule.ts

+
+    public walk(sourceFile: ts.SourceFile): void {
+        sourceFile.statements
+            .filter(isImportDeclaration)


what about ImportEqualsDeclaration where isExternalModuleReference(statement.moduleReference)? For example:

import fs = require('fs');

ajafff · 2017-07-24T19:15:20Z

src/rules/groupedImportsRule.ts

+        }
+    }
+
+    private static getImportPath(statement: ts.Statement): string {


getImportPath(statement: ts.ImportDeclaration): string { if (isTextualLiteral(statement.moduleSpecifier)) { return statement.moduleSpecifier.text; } // TODO handle invalid grammar like `import foo from 'a'+foo+'b';` }

Added. In what case the isTextualLiteral returns false? Your example seems to be processed as import foo from 'a'.

ajafff · 2017-07-24T19:18:53Z

src/rules/groupedImportsRule.ts

+}
+
+class Walker extends Lint.AbstractWalker<Lint.IOptions> {
+    private lastImportStatement: ImportStatement;


: ImportStatement | undefined

Is this really necessary? It adds a lot of complexity and additional testing throughout the rest of the code.

That's just my preference for documentation purposes. It helps someone else (or even you) to understand how the rule works when he needs to change something.
There's no need for any additional checks. In the places where lastImportStatement can be undefined, you already have a check. In all other places you can simply assert that it's never undefined, e.g. this.lastImportStatement!.type

Ah great! Didn't know that was possible.

ajafff · 2017-07-24T19:21:27Z

src/rules/groupedImportsRule.ts

+        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());


I guess this won't work for

import foo from 'foo';import bar from 'bar'; // note the missing space after the first semicolon

True, updated the fixes.

ajafff · 2017-07-24T19:22:02Z

src/rules/groupedImportsRule.ts

+
+    private checkImportStatement(importStatement: ImportStatement) {
+        if (importStatement.type === this.lastImportStatement.type) {
+            if (importStatement.lineStart !== this.lastImportStatement.lineEnd + 1) {


should probably be > instead of !==, see comment below

ajafff · 2017-07-24T19:23:59Z

src/rules/groupedImportsRule.ts

+        } 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) {


ajafff · 2017-07-24T19:24:29Z

src/rules/groupedImportsRule.ts

+                    this.lastImportStatement.statement.getEnd() + 1, importStatement.statement.getStart());
+                this.addFailureAtNode(importStatement.statement, Rule.IMPORT_SOURCES_SEPARATED, replacement);
+            }
+        } else if (importStatement.type.valueOf() < this.lastImportStatement.type.valueOf()) {


why the valueOf()?

ajafff · 2017-07-24T19:26:22Z

src/rules/groupedImportsRule.ts

+            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);


ts.sys.newLine does not always match the line endings in the current file.
Have a look at eoflineRule.ts to see how the line ending is inferred there.

Thanks. I've updated this.

ajafff · 2017-07-24T19:33:09Z

src/rules/groupedImportsRule.ts

+        });
+        return Lint.Replacement.replaceFromTo(
+            importStatements[0].getStart(),
+            importStatements[importStatements.length - 1].getEnd(),


I guess that will break code where there's another statement between imports:

import bar from './bar'; let baz = bar.baz; import foo from 'foo';

eriktim · 2017-07-25T14:06:43Z

Thanks for your first remarks @ajafff.

I've fixed some of the issues. But three issues need some further thinking first:

Preserve comments.
How to deal with import fs = require('fs');-like statements?
How to deal with statements between imports?

Edit: I've thinking about the comments/additional statements. If you ask me they should all be put AFTER the imports (without deleting any of them).

ajafff · 2017-07-27T18:47:43Z

@trenneman

How to deal with import fs = require('fs');-like statements?

It's probably fine to ignore them like ordered-imports does. This kind of import is only there for backwards compatibility with pre-ES6 modules anyway.

I've thinking about the comments/additional statements. If you ask me they should all be put AFTER the imports (without deleting any of them).

That would be the best way to go. Maybe someone will complain about the wrong comment position after fixing. But until then we should keep the rule simple.
If you have trouble implementing the special handling for statements between imports, you can also choose to not implement it and just don't fix in such cases. IMO that code path is not likely to be hit often.

eriktim · 2017-07-28T12:28:14Z

@ajafff Please have a look at the updated version. The walker now quits after the first incorrect import and creates a fix for all imports. The imports are removed and re-inserted in the correct order. This ensures no existing code (comments or statements) is lost.

ajafff

This looks really good.
I added some suggestions. And the insertion position for the fix needs to be fixed as mentioned in the comments.

ajafff · 2017-07-28T20:34:27Z

src/rules/groupedImportsRule.ts

+    public walk(sourceFile: ts.SourceFile): void {
+        const importsStatements = sourceFile.statements
+            .filter(isImportDeclaration)
+            .map(this.toImportStatement);


use .map(this.toImportStatement, this). Same for .find(this.isBadlyPositioned, this) below.
That way you can make both methods "real" methods (no need for an arrow function)

ajafff · 2017-07-28T20:40:05Z

src/rules/groupedImportsRule.ts

+    private createFix(importStatements: ImportStatement[]): Lint.Fix {
+        const newLine = this.getEofChar(this.sourceFile);
+        const imports = this.getOrderedImports(importStatements);
+        const addition = Lint.Replacement.appendText(0, imports.join(newLine));


Instead of 0 consider using the start of the first import found as insertion position.

If you use 0, you'll break source files that contain a shebang (e.g. #! env node) in the first line.

ajafff · 2017-07-28T20:42:32Z

src/rules/groupedImportsRule.ts

+        return char === undefined ? false : Lint.isWhiteSpace(char.charCodeAt(0));
+    }
+
+    private getEofChar(sourceFile: ts.SourceFile): string {


ajafff · 2017-07-28T20:44:16Z

src/rules/groupedImportsRule.ts

+            if (imps.length == 0) {
+                return arr;
+            }
+            return arr.concat(imps.map((imp) => imp.statement.getText()), "");


nit: consider passing sourceFile as parameter to .getText() and .getStart() for performance reasons.
Though that's not as relevant here, because it's only used in case of a lint error (which should not be the hot path).

* 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.

And preserve all statements.

* Stop using arrow function methods; * Insert fix at start of first import statement (and only remove up to the end of the last import statement). * Performance improvements; * Fix eol typo.

eriktim · 2017-08-02T11:38:30Z

Thanks @ajafff. I've processed your comments. getRangeIncludingWhitespace now only removes from the beginning of the first to the end of the last import statement.

I've also rebased on master. Let me know when I should squash as well.

adidahiya

overall this looks pretty good. I left a few comments; the documentation ones are the most important. also it looks like you need to resolve one file conflict.

what do you think about integrating this behavior into the existing ordered-imports rule as a new rule option? it does in fact enforce a specific ordering of the groups, so I think it would make sense to users.

pros for integrating into the existing rule:

In general I would prefer to avoid bloating the core rules list whenever possible. It's already pretty long.

cons:

Users can't disable this lint rule inline (// tslint:disable) independent of other behavior of ordered-imports. However, I don't think this is a common use case -- users are much more likely to disable the whole rule or rule option for their project. Inline disabling is much more important for rules like max-line-length, no-unused-variable, etc.

also, notes for PR process:

you don't need to rebase (prefer merging the base branch)
you don't need to squash (we use github squash merges)

adidahiya · 2017-08-10T17:04:06Z

src/rules/groupedImportsRule.ts

+    /* tslint:disable:object-literal-sort-keys */
+    public static metadata: Lint.IRuleMetadata = {
+        ruleName: "grouped-imports",
+        description: "Separate import groups by blank lines.",


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 of IRuleMetadata.

adidahiya · 2017-08-10T17:07:05Z

src/rules/groupedImportsRule.ts

+        hasFix: true,
+        options: {},
+        optionExamples: [true],
+        type: "style",


I would actually argue that this falls into "maintainability"

adidahiya · 2017-08-10T17:08:00Z

src/rules/groupedImportsRule.ts

+        return false;
+    }
+
+    private toImportStatement(statement: ts.ImportDeclaration): ImportStatement {


nit: slightly better name would be importDeclarationToStatement

eriktim · 2017-08-14T11:53:36Z

Thanks @adidahiya

I actually think it makes sense to merge it with the ordered-imports rule. Especially, because I already use it on top of that one. It will take me some time to understand that rule as it it rather large, but I think I can manage that. Shall I update this PR with a proposal?

adidahiya · 2017-08-14T19:11:55Z

@trenneman I think opening a new PR would be best (link to it from this one).

eriktim · 2017-08-17T15:25:30Z

Continued in #3138 .

eriktim mentioned this pull request Jul 21, 2017

New rule: import-ordering #771

Closed

eriktim force-pushed the grouped-imports branch from 6605a4d to f9c1e3e Compare July 21, 2017 15:02

ajafff suggested changes Jul 24, 2017

View reviewed changes

eriktim force-pushed the grouped-imports branch 2 times, most recently from 965acdf to f9b5d0b Compare July 26, 2017 14:39

eriktim force-pushed the grouped-imports branch from 1a76f91 to cb2aa6e Compare July 28, 2017 12:24

eriktim force-pushed the grouped-imports branch 2 times, most recently from c44dbb9 to b19f3ae Compare July 28, 2017 12:46

ajafff approved these changes Jul 28, 2017

View reviewed changes

eriktim added 4 commits August 2, 2017 13:22

Add new rule: grouped-imports

ab50981

Process some first review comments

d985f8f

* 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.

Rewrite to always use a single fix

c3d74cc

And preserve all statements.

Process additional review comments

83f1960

* Stop using arrow function methods; * Insert fix at start of first import statement (and only remove up to the end of the last import statement). * Performance improvements; * Fix eol typo.

eriktim force-pushed the grouped-imports branch from b19f3ae to 83f1960 Compare August 2, 2017 11:34

adidahiya suggested changes Aug 10, 2017

View reviewed changes

adidahiya self-assigned this Aug 10, 2017

eriktim closed this Aug 17, 2017

Add new rule: grouped-imports #3064

Add new rule: grouped-imports #3064

Conversation

eriktim commented Jul 21, 2017

PR checklist

Overview of change:

Is there anything you'd like reviewers to focus on?

CHANGELOG.md entry:

palantirtech commented Jul 21, 2017

ajafff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajafff Jul 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eriktim commented Jul 25, 2017 • edited Loading

ajafff commented Jul 27, 2017

eriktim commented Jul 28, 2017 • edited Loading

ajafff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eriktim commented Aug 2, 2017

adidahiya left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eriktim commented Aug 14, 2017

adidahiya commented Aug 14, 2017

eriktim commented Aug 17, 2017

ajafff Jul 24, 2017 •

edited

Loading

eriktim commented Jul 25, 2017 •

edited

Loading

eriktim commented Jul 28, 2017 •

edited

Loading

adidahiya left a comment •

edited

Loading