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

* 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

Author: Radek Benkel

Diff for: 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 for: 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 for: 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 for: 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',
         },
       ],
     }),