Add import-spacing rule #1935

mohsen1 · 2016-12-25T20:03:36Z

This PR adds import-spacing rule which enforces proper spacing in import declarations.

This is implementation of #1584

PR checklist

New feature, bugfix, or enhancement
- Includes tests
Documentation update

mohsen1 · 2016-12-28T19:24:29Z

I don't see any failures in the output but my tests are failing. Can you help?

IllusionMH · 2016-12-29T00:21:57Z

@mohsen1 I'm unable to create PR to your fork.

You can find changes that allowed me to fix test in commit f37c74d

mohsen1 · 2016-12-29T00:27:33Z

@IllusionMH thanks for looking into it.
What nil means in there?

jkillian · 2016-12-29T00:36:49Z

~nil means that the error starts there but has zero width. In this case, it probably means that the error indicates that it starts after the last character of the line (where the line break character would be I suppose). In general it's nicer to avoid outputting errors like this if possible.

IllusionMH · 2016-12-29T00:42:33Z

@mohsen1 ~nil is described in lines.ts
In this case error starts on next character after } (in second case) and this character is line break - which doesn't have length and can't be marked with regular ~.

mohsen1 · 2016-12-29T00:59:55Z

@jkillian I think this is a valid use case

mohsen1 · 2016-12-29T01:07:15Z

src/rules/importSpacingRule.ts

+        typescriptOnly: false,
+    };
+
+    public static ADD_SPACE_AFTER_IMPORT = "Add space after import";


Please feel free to correct my error wordings here.

How about some quotes: Add space after 'import', similarly, below

nchen63

Looks pretty good. Some minor nits. Good tests

nchen63 · 2017-01-01T02:55:06Z

src/rules/importSpacingRule.ts

@@ -0,0 +1,124 @@
+/**
+ * @license
+ * Copyright 2013 Palantir Technologies, Inc.


2016 (or 2017 if applicable)

nchen63 · 2017-01-01T02:56:51Z

src/rules/importSpacingRule.ts

+        typescriptOnly: false,
+    };
+
+    public static ADD_SPACE_AFTER_IMPORT = "Add space after import";


How about some quotes: Add space after 'import', similarly, below

nchen63 · 2017-01-01T02:58:11Z

src/rules/importSpacingRule.ts

+    public static ADD_SPACE_AFTER_STAR = "Add space after *";
+    public static TOO_MANY_SPACES_AFTER_STAR = "Too many spaces after *";
+    public static ADD_SPACE_AFTER_FROM = "Add space after from";
+    public static TOO_MANY_SPACES_AFTER_FROM = "Too many spaces after from";


Too many space after 'from', above and below

nchen63 · 2017-01-01T02:58:48Z

src/rules/importSpacingRule.ts

+    public static ADD_SPACE_AFTER_IMPORT = "Add space after import";
+    public static TOO_MANY_SPACES_AFTER_IMPORT = "Too many spaces after import";
+    public static ADD_SPACE_AFTER_STAR = "Add space after *";
+    public static TOO_MANY_SPACES_AFTER_STAR = "Too many spaces after *";


Too many spaces after '*', and above

nchen63 · 2017-01-01T03:01:17Z

src/rules/importSpacingRule.ts

+}
+
+class ImportStatementWalker extends Lint.RuleWalker {
+    private static IMPORT_KEYWORD_LENGTH = 6; // "import".length;


you can just define it as "import".length and omit the comment. It is probably even more clear to not define this variable and just just "import".length everywhere

Isn't it more performance friendly to do this?

The perf difference is not measurable. See the jsperf run https://jsperf.com/str-length

nchen63 · 2017-01-01T03:24:45Z

src/rules/importSpacingRule.ts

+        if ((nodeStart + ImportStatementWalker.IMPORT_KEYWORD_LENGTH + 1) < moduleSpecifierStart) {
+            this.addFailure(this.createFailure(nodeStart, moduleSpecifierStart - nodeStart, Rule.TOO_MANY_SPACES_AFTER_IMPORT));
+        }
+        if ((nodeStart + ImportStatementWalker.IMPORT_KEYWORD_LENGTH) === moduleSpecifierStart) {


nchen63 · 2017-01-01T03:39:58Z

src/rules/importSpacingRule.ts

+            if (/from$/.test(fromString)) {
+                this.addFailure(this.createFailure(importClauseEnd, fromString.length, Rule.ADD_SPACE_AFTER_FROM));
+            }
+            if (/from\s{2,}$/.test(fromString)) {


nchen63 · 2017-01-01T03:42:09Z

src/rules/importSpacingRule.ts

+            if (/^\s{2,}from/.test(fromString)) {
+                this.addFailure(this.createFailure(importClauseEnd, fromString.length, Rule.TOO_MANY_SPACES_BEFORE_FROM));
+            }
+            if (/^from/.test(fromString)) {


nchen63 · 2017-01-01T20:21:09Z

@mohsen1 thanks!

adidahiya · 2017-01-03T18:00:38Z

The banning of line breaks in import statements is problematic here. We need to prescribe a way to make this rule play nicely with max-line-length out of the box. Right now we don't have an option for the line length rule to skip import statements (#1872 (comment)). IMO I would prefer to keep the max-line-length rule in effect for import statements and allow multiline import statements in the default TSLint config.

import {
    RuleWalker,
    RuleFailure,
    ...
} from "tslint";

mohsen1 · 2017-01-03T18:04:31Z

It does allow line breaks in destructuring import clause. although it won't let a very long module specifier like this:

import * as veryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongName from './very-very-very-very-very-very-very-very-very-long-name';

But a line break might not solve the issue if the specifier itself is longer than max lenght.

adidahiya · 2017-01-03T18:15:54Z

Ah, cool, sorry I missed that. This implementation looks reasonable then.

Mohsen Azimi added 2 commits December 25, 2016 12:02

Make import-spacing rule

7883f69

Handle spacing between * and as in visitNamespaceImport

4fc6bd6

mohsen1 force-pushed the import-spacing branch from 1183703 to 4fc6bd6 Compare December 25, 2016 20:19

Mohsen Azimi added 7 commits December 25, 2016 14:20

Enforce space after import

e9be547

Enforce space after from

3652af3

Handle side effect module imports

659ecb1

Enforce space before and after "from"

9a3c767

Add line break failures

2bb92f5

Add more failing tests for line breaks

3be5f03

Enforce line breaks in import statements

9d22b21

mohsen1 changed the title ~~WIP: Add import-spacing rule~~ Add import-spacing rule Dec 28, 2016

Merge branch 'master' of github.com:palantir/tslint into import-spacing

e1e1c4e

Add nil for tests

8e055db

mohsen1 commented Dec 29, 2016

View reviewed changes

Mohsen Azimi added 2 commits December 31, 2016 18:16

Merge branch 'master' into import-spacing

ce6a1d3

Generate docs

afc57ad

nchen63 suggested changes Jan 1, 2017

View reviewed changes

Mohsen Azimi added 3 commits January 1, 2017 11:55

Fix wording

8854c0a

Remove import length variable

3c5e081

Add else if instead of if for places it makes sense

9a53021

nchen63 merged commit 7d9f7e7 into palantir:master Jan 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add import-spacing rule #1935

Add import-spacing rule #1935

mohsen1 commented Dec 25, 2016 •

edited

mohsen1 commented Dec 28, 2016

IllusionMH commented Dec 29, 2016

mohsen1 commented Dec 29, 2016

jkillian commented Dec 29, 2016

IllusionMH commented Dec 29, 2016

mohsen1 commented Dec 29, 2016

mohsen1 Dec 29, 2016

nchen63 Jan 1, 2017

nchen63 left a comment

nchen63 Jan 1, 2017

nchen63 Jan 1, 2017

nchen63 Jan 1, 2017

nchen63 Jan 1, 2017

nchen63 Jan 1, 2017

mohsen1 Jan 1, 2017

nchen63 Jan 1, 2017

nchen63 Jan 1, 2017

nchen63 Jan 1, 2017

nchen63 Jan 1, 2017

nchen63 commented Jan 1, 2017

adidahiya commented Jan 3, 2017

mohsen1 commented Jan 3, 2017

adidahiya commented Jan 3, 2017

Add import-spacing rule #1935

Add import-spacing rule #1935

Conversation

mohsen1 commented Dec 25, 2016 • edited

PR checklist

mohsen1 commented Dec 28, 2016

IllusionMH commented Dec 29, 2016

mohsen1 commented Dec 29, 2016

jkillian commented Dec 29, 2016

IllusionMH commented Dec 29, 2016

mohsen1 commented Dec 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nchen63 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

Choose a reason for hiding this comment

nchen63 commented Jan 1, 2017

adidahiya commented Jan 3, 2017

mohsen1 commented Jan 3, 2017

adidahiya commented Jan 3, 2017

mohsen1 commented Dec 25, 2016 •

edited