Fix order's newlines-between edge cases (import-js#339)

Radek Benkel · benmosher · commit 0a918b1ba138 · 2016-05-14T09:47:37.000-04:00
* Change required lines amount to minimum 1 (instead of exactly 1)

to be at least one, not exactly one

Before it was 1. To prevent multiple lines between imports, core
`no-multiple-empty-lines` rule can be used.

* Fix `order`s `newline-between` for multiline imports

* Ignore unassigned imports while detecting empty lines

Previously it was calculated based on `loc` property of nodes. Counting
REAL empty lines is less error-prone.

* Fix `require` in object properties
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,6 +7,9 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel
 ### Fixed
 - `export * from 'foo'` now properly ignores a `default` export from `foo`, if any. ([#328]/[#332], thanks [@jkimbo])
   This impacts all static analysis of imported names. ([`default`], [`named`], [`namespace`], [`export`])
+- Make [`order`]'s `newline-between` option handle multiline import statements ([#313], thanks [@singles])
+- Make [`order`]'s `newline-between` option handle not assigned import statements ([#313], thanks [@singles])
+- Make [`order`]'s `newline-between` option ignore `require` statements inside object literals ([#313], thanks [@singles])
 
 ## [1.8.0] - 2016-05-11
 ### Added
diff --git a/docs/rules/order.md b/docs/rules/order.md
@@ -98,7 +98,7 @@ You can set the options like this:
 Enforces or forbids new lines between import groups:
 
 - If omitted, assertion messages will be neither enforced nor forbidden.
-- If set to `always`, a new line between each group will be enforced, and new lines inside a group will be forbidden.
+- If set to `always`, at least one new line between each group will be enforced, and new lines inside a group will be forbidden. To prevent multiple lines between imports, core `no-multiple-empty-lines` rule can be used.
 - If set to `never`, no new lines are allowed in the entire import section.
 
 With the default group setting, the following will be invalid:
diff --git a/src/rules/order.js b/src/rules/order.js
@@ -110,28 +110,33 @@ function convertGroupsToRanks(groups) {
 }
 
 function makeNewlinesBetweenReport (context, imported, newlinesBetweenImports) {
-  const getLineDifference = (currentImport, previousImport) => {
-    return currentImport.node.loc.start.line - previousImport.node.loc.start.line
+  const getNumberOfEmptyLinesBetween = (currentImport, previousImport) => {
+    const linesBetweenImports = context.getSourceCode().lines.slice(
+      previousImport.node.loc.end.line,
+      currentImport.node.loc.start.line - 1
+    )
+
+    return linesBetweenImports.filter((line) => !line.trim().length).length
   }
   let previousImport = imported[0]
 
   imported.slice(1).forEach(function(currentImport) {
     if (newlinesBetweenImports === 'always') {
       if (currentImport.rank !== previousImport.rank
-        && getLineDifference(currentImport, previousImport) !== 2)
+        && getNumberOfEmptyLinesBetween(currentImport, previousImport) === 0)
       {
         context.report(
-          previousImport.node, 'There should be one empty line between import groups'
+          previousImport.node, 'There should be at least one empty line between import groups'
         )
       } else if (currentImport.rank === previousImport.rank
-        && getLineDifference(currentImport, previousImport) >= 2)
+        && getNumberOfEmptyLinesBetween(currentImport, previousImport) > 0)
       {
         context.report(
           previousImport.node, 'There should be no empty line within import group'
         )
       }
     } else {
-      if (getLineDifference(currentImport, previousImport) > 1) {
+      if (getNumberOfEmptyLinesBetween(currentImport, previousImport) > 0) {
         context.report(previousImport.node, 'There should be no empty line between import groups')
       }
     }
@@ -191,10 +196,12 @@ module.exports = function importOrderRule (context) {
     FunctionExpression: incrementLevel,
     ArrowFunctionExpression: incrementLevel,
     BlockStatement: incrementLevel,
+    ObjectExpression: incrementLevel,
     'FunctionDeclaration:exit': decrementLevel,
     'FunctionExpression:exit': decrementLevel,
     'ArrowFunctionExpression:exit': decrementLevel,
     'BlockStatement:exit': decrementLevel,
+    'ObjectExpression:exit': decrementLevel,
   }
 }
 
diff --git a/tests/src/rules/order.js b/tests/src/rules/order.js
@@ -167,8 +167,11 @@ ruleTester.run('order', rule, {
         var index = require('./');
         var path = require('path');
 
+
+
         var sibling = require('./foo');
 
+
         var relParent1 = require('../foo');
         var relParent3 = require('../');
         var async = require('async');
@@ -206,6 +209,121 @@ ruleTester.run('order', rule, {
         },
       ],
     }),
+    // Option newlines-between: 'always' with multiline imports #1
+    test({
+      code: `
+        import path from 'path';
+
+        import {
+            I,
+            Want,
+            Couple,
+            Imports,
+            Here
+        } from 'bar';
+        import external from 'external'
+      `,
+      options: [{ 'newlines-between': 'always' }]
+    }),
+    // Option newlines-between: 'always' with multiline imports #2
+    test({
+      code: `
+        import path from 'path';
+        import net
+          from 'net';
+
+        import external from 'external'
+      `,
+      options: [{ 'newlines-between': 'always' }]
+    }),
+    // Option newlines-between: 'always' with multiline imports #3
+    test({
+      code: `
+        import foo
+          from '../../../../this/will/be/very/long/path/and/therefore/this/import/has/to/be/in/two/lines';
+
+        import bar
+          from './sibling';
+      `,
+      options: [{ 'newlines-between': 'always' }]
+    }),
+    // Option newlines-between: 'always' with not assigned import #1
+    test({
+      code: `
+        import path from 'path';
+
+        import 'loud-rejection';
+        import 'something-else';
+
+        import _ from 'lodash';
+      `,
+      options: [{ 'newlines-between': 'always' }]
+    }),
+    // Option newlines-between: 'never' with not assigned import #2
+    test({
+      code: `
+        import path from 'path';
+        import 'loud-rejection';
+        import 'something-else';
+        import _ from 'lodash';
+      `,
+      options: [{ 'newlines-between': 'never' }]
+    }),
+    // Option newlines-between: 'always' with not assigned require #1
+    test({
+      code: `
+        var path = require('path');
+
+        require('loud-rejection');
+        require('something-else');
+
+        var _ = require('lodash');
+      `,
+      options: [{ 'newlines-between': 'always' }]
+    }),
+    // Option newlines-between: 'never' with not assigned require #2
+    test({
+      code: `
+        var path = require('path');
+        require('loud-rejection');
+        require('something-else');
+        var _ = require('lodash');
+      `,
+      options: [{ 'newlines-between': 'never' }]
+    }),
+    // Option newlines-between: 'never' should ignore nested require statement's #1
+    test({
+      code: `
+        var some = require('asdas');
+        var config = {
+          port: 4444,
+          runner: {
+            server_path: require('runner-binary').path,
+            
+            cli_args: {
+                'webdriver.chrome.driver': require('browser-binary').path
+            }
+          }
+        }
+      `,
+      options: [{ 'newlines-between': 'never' }]
+    }),
+    // Option newlines-between: 'always' should ignore nested require statement's #2
+    test({
+      code: `
+        var some = require('asdas');
+        var config = {
+          port: 4444,
+          runner: {
+            server_path: require('runner-binary').path,
+            cli_args: {
+                'webdriver.chrome.driver': require('browser-binary').path
+            }
+          }
+        }
+      `,
+      options: [{ 'newlines-between': 'always' }]
+    }),
   ],
   invalid: [
     // builtin before external module (require)
@@ -517,23 +635,24 @@ ruleTester.run('order', rule, {
       errors: [
         {
           line: 4,
-          message: 'There should be one empty line between import groups',
+          message: 'There should be at least one empty line between import groups',
         },
         {
           line: 5,
-          message: 'There should be one empty line between import groups',
+          message: 'There should be at least one empty line between import groups',
         },
       ],
     }),
-    //Option newlines-between: 'always' should report too many empty lines between import groups
+    //Option newlines-between: 'always' should report unnecessary empty lines space between import groups
     test({
       code: `
         var fs = require('fs');
-        var index = require('./');
-
 
+        var path = require('path');
+        var index = require('./');
 
         var sibling = require('./foo');
+
         var async = require('async');
       `,
       options: [
@@ -547,40 +666,45 @@ ruleTester.run('order', rule, {
       ],
       errors: [
         {
-          line: 3,
-          message: 'There should be one empty line between import groups',
+          line: 2,
+          message: 'There should be no empty line within import group',
+        },
+        {
+          line: 7,
+          message: 'There should be no empty line within import group',
         },
       ],
     }),
-    //Option newlines-between: 'always' should report unnecessary empty lines space between import groups
+    // Option newlines-between: 'never' should report unnecessary empty lines when using not assigned imports
     test({
       code: `
-        var fs = require('fs');
-
-        var path = require('path');
-        var index = require('./');
-
-        var sibling = require('./foo');
+        import path from 'path';
+        import 'loud-rejection';
 
-        var async = require('async');
+        import 'something-else';
+        import _ from 'lodash';
       `,
-      options: [
+      options: [{ 'newlines-between': 'never' }],
+      errors: [
         {
-          groups: [
-            ['builtin', 'index'],
-            ['sibling', 'parent', 'external']
-          ],
-          'newlines-between': 'always',
+          line: 2,
+          message: 'There should be no empty line between import groups',
         },
       ],
+    }),
+    // Option newlines-between: 'always' should report missing empty lines when using not assigned imports
+    test({
+      code: `
+        import path from 'path';
+        import 'loud-rejection';
+        import 'something-else';
+        import _ from 'lodash';
+      `,
+      options: [{ 'newlines-between': 'always' }],
       errors: [
         {
           line: 2,
-          message: 'There should be no empty line within import group',
-        },
-        {
-          line: 7,
-          message: 'There should be no empty line within import group',
+          message: 'There should be at least one empty line between import groups',
         },
       ],
     }),

Original file line number	Diff line number	Diff line change
`@@ -110,28 +110,33 @@ function convertGroupsToRanks(groups) {`
`110`	`110`	`}`
`111`	`111`
`112`	`112`	`function makeNewlinesBetweenReport (context, imported, newlinesBetweenImports) {`
`113`		`- const getLineDifference = (currentImport, previousImport) => {`
`114`		`- return currentImport.node.loc.start.line - previousImport.node.loc.start.line`
	`113`	`+ const getNumberOfEmptyLinesBetween = (currentImport, previousImport) => {`
	`114`	`+ const linesBetweenImports = context.getSourceCode().lines.slice(`
	`115`	`+ previousImport.node.loc.end.line,`
	`116`	`+ currentImport.node.loc.start.line - 1`
	`117`	`+ )`
	`118`	`+`
	`119`	`+ return linesBetweenImports.filter((line) => !line.trim().length).length`
`115`	`120`	`}`
`116`	`121`	`let previousImport = imported[0]`
`117`	`122`
`118`	`123`	`imported.slice(1).forEach(function(currentImport) {`
`119`	`124`	`if (newlinesBetweenImports === 'always') {`
`120`	`125`	`if (currentImport.rank !== previousImport.rank`
`121`		`- && getLineDifference(currentImport, previousImport) !== 2)`
	`126`	`+ && getNumberOfEmptyLinesBetween(currentImport, previousImport) === 0)`
`122`	`127`	`{`
`123`	`128`	`context.report(`
`124`		`- previousImport.node, 'There should be one empty line between import groups'`
	`129`	`+ previousImport.node, 'There should be at least one empty line between import groups'`
`125`	`130`	`)`
`126`	`131`	`} else if (currentImport.rank === previousImport.rank`
`127`		`- && getLineDifference(currentImport, previousImport) >= 2)`
	`132`	`+ && getNumberOfEmptyLinesBetween(currentImport, previousImport) > 0)`
`128`	`133`	`{`
`129`	`134`	`context.report(`
`130`	`135`	`previousImport.node, 'There should be no empty line within import group'`
`131`	`136`	`)`
`132`	`137`	`}`
`133`	`138`	`} else {`
`134`		`- if (getLineDifference(currentImport, previousImport) > 1) {`
	`139`	`+ if (getNumberOfEmptyLinesBetween(currentImport, previousImport) > 0) {`
`135`	`140`	`context.report(previousImport.node, 'There should be no empty line between import groups')`
`136`	`141`	`}`
`137`	`142`	`}`
`@@ -191,10 +196,12 @@ module.exports = function importOrderRule (context) {`
`191`	`196`	`FunctionExpression: incrementLevel,`
`192`	`197`	`ArrowFunctionExpression: incrementLevel,`
`193`	`198`	`BlockStatement: incrementLevel,`
	`199`	`+ ObjectExpression: incrementLevel,`
`194`	`200`	`'FunctionDeclaration:exit': decrementLevel,`
`195`	`201`	`'FunctionExpression:exit': decrementLevel,`
`196`	`202`	`'ArrowFunctionExpression:exit': decrementLevel,`
`197`	`203`	`'BlockStatement:exit': decrementLevel,`
	`204`	`+ 'ObjectExpression:exit': decrementLevel,`
`198`	`205`	`}`
`199`	`206`	`}`
`200`	`207`