Apply first round of PR feedback

* Change `sort-order` option to `sort`
* Rename `alphabetical` function to `isAlphabetized`
* Change `isAlphabetized` to include equality comparison, to handle duplicate imports
* Add test cases to verify alphabetical sort is applied correctly to duplicate imports
* Simplify `isAlphabetized` with guard clause
* Make explicit comparison for `array.length === 0`
* Have a single check of `sort` option before triggering `makeOutOfAlphabeticalOrderReport`, do not pass as parameter

docs/rules/order.md

@@ -141,7 +141,7 @@ import index from './';
 import sibling from './foo';
 ```
 
-### `sort-order: [ignore|alphabetical]`:
+### `sort: [ignore|alphabetical]`:
 
 
 Enforces alphabetical sorting within import groups:
@@ -152,7 +152,7 @@ Enforces alphabetical sorting within import groups:
 With the default group setting, the following will be invalid:
 
 ```js
-/* eslint import/order: ["error", {"sort-order": "alphabetical"}] */
+/* eslint import/order: ["error", {"sort": "alphabetical"}] */
 import path from 'path';
 import fs from 'fs';
 import index from './';
@@ -162,15 +162,15 @@ import sibling from './foo';
 while this will be valid:
 
 ```js
-/* eslint import/order: ["error", {"sort-order": "alphabetical"}] */
+/* eslint import/order: ["error", {"sort": "alphabetical"}] */
 import fs from 'fs';
 import path from 'path';
 import index from './';
 import sibling from './foo';
 ```
 
 ```js
-/* eslint import/order: ["error", {"sort-order": "ignore"}] */
+/* eslint import/order: ["error", {"sort": "ignore"}] */
 import path from 'path';
 import fs from 'fs';
 import index from './';

src/rules/order.js

@@ -17,14 +17,11 @@ function reverse(array) {
   }).reverse()
 }
 
-function alphabetical(firstName, secondName, direction) {
+function isAlphabetized(firstString, secondString, direction) {
   if (direction === 'a-z') {
-    return firstName < secondName
-  } else if (direction === 'z-a') {
-    return firstName > secondName
-  } else {
-    throw new Error('Invalid alphabetical direction' + direction)
+    return firstString <= secondString
   }
+  return firstString >= secondString
 }
 
 function findOutOfOrder(imported) {
@@ -41,8 +38,8 @@ function findOutOfOrder(imported) {
   })
 }
 
-function findOutOfAlphabeticalOrder(imported, sortOrder, direction) {
-  if (imported.length === 0 || sortOrder === 'ignore') {
+function findOutOfAlphabeticalOrder(imported, direction) {
+  if (imported.length === 0) {
     return []
   }
   let maxSeenRankNode = imported[0]
@@ -53,13 +50,13 @@ function findOutOfAlphabeticalOrder(imported, sortOrder, direction) {
       // New group, reset max alphabetical node
       maxSeenAlphabeticalNode = importedModule
     }
-    const resAlphabetical = alphabetical(
-      importedModule.name,
+    const resAlphabetical = !isAlphabetized(
       maxSeenAlphabeticalNode.name,
+      importedModule.name,
       direction
     )
     const reversedDirection = direction.split('').reverse().join('')
-    if (alphabetical(importedModule.name, maxSeenAlphabeticalNode.name, reversedDirection)) {
+    if (isAlphabetized(importedModule.name, maxSeenAlphabeticalNode.name, reversedDirection)) {
       maxSeenAlphabeticalNode = importedModule
     }
     return resAlphabetical
@@ -79,7 +76,10 @@ function reportOutOfOrder(context, imported, outOfOrder, order) {
 function reportOutOfAlphabeticalOrder(context, imported, outOfOrder, order, direction) {
   outOfOrder.forEach(function (imp) {
     const found = imported.find(function hasHigherAlphabeticalOrder(importedItem) {
-      return alphabetical(imp.name, importedItem.name, direction) && importedItem.rank === imp.rank
+      const alphabetized = isAlphabetized(imp.name, importedItem.name, direction)
+      const sameGroup = (importedItem.rank === imp.rank)
+      const notDuplicateImport = (imp.name !== importedItem.name)
+      return alphabetized && sameGroup && notDuplicateImport
     })
     context.report(imp.node, '`' + imp.name + '` import should occur ' + order +
       ' import of `' + found.name + '`')
@@ -88,7 +88,7 @@ function reportOutOfAlphabeticalOrder(context, imported, outOfOrder, order, dire
 
 function makeOutOfOrderReport(context, imported) {
   const outOfOrder = findOutOfOrder(imported)
-  if (!outOfOrder.length) {
+  if (outOfOrder.length === 0) {
     return
   }
   // There are things to report. Try to minimize the number of reported errors.
@@ -101,14 +101,14 @@ function makeOutOfOrderReport(context, imported) {
   reportOutOfOrder(context, imported, outOfOrder, 'before')
 }
 
-function makeOutOfAlphabeticalOrderReport(context, imported, sortOrder) {
-  const outOfOrder = findOutOfAlphabeticalOrder(imported, sortOrder, 'a-z')
-  if (!outOfOrder.length) {
+function makeOutOfAlphabeticalOrderReport(context, imported) {
+  const outOfOrder = findOutOfAlphabeticalOrder(imported, 'a-z')
+  if (outOfOrder.length === 0) {
     return
   }
   // There are things to report. Try to minimize the number of reported errors.
   const reversedImported = reverse(imported)
-  const reversedOrder = findOutOfAlphabeticalOrder(reversedImported, sortOrder, 'z-a')
+  const reversedOrder = findOutOfAlphabeticalOrder(reversedImported, 'z-a')
   if (reversedOrder.length < outOfOrder.length) {
     reportOutOfAlphabeticalOrder(context, reversedImported, reversedOrder, 'after', 'z-a')
     return
@@ -218,7 +218,7 @@ module.exports = {
           'newlines-between': {
             enum: [ 'ignore', 'always', 'never' ],
           },
-          'sort-order': {
+          'sort': {
             enum: [ 'alphabetical', 'ignore' ],
           },
         },
@@ -230,7 +230,7 @@ module.exports = {
   create: function importOrderRule (context) {
     const options = context.options[0] || {}
     const newlinesBetweenImports = options['newlines-between'] || 'ignore'
-    const sortOrder = options['sort-order'] || 'ignore'
+    const sortOrder = options['sort'] || 'ignore'
     let ranks
 
     try {
@@ -269,7 +269,9 @@ module.exports = {
       },
       'Program:exit': function reportAndReset() {
         makeOutOfOrderReport(context, imported)
-        makeOutOfAlphabeticalOrderReport(context, imported, sortOrder)
+        if (sortOrder === 'alphabetical') {
+          makeOutOfAlphabeticalOrderReport(context, imported)
+        }
 
         if (newlinesBetweenImports !== 'ignore') {
           makeNewlinesBetweenReport(context, imported, newlinesBetweenImports)

tests/src/rules/order.js

@@ -382,7 +382,15 @@ ruleTester.run('order', rule, {
         import fs from 'fs';
         import path from 'path';
       `,
-      options: [{ 'sort-order': 'alphabetical' }],
+      options: [{ 'sort': 'alphabetical' }],
+    }),
+    // Alphabetical order with duplicate import
+    test({
+      code: `
+        import fs from 'fs';
+        import fs from 'fs';
+      `,
+      options: [{ 'sort': 'alphabetical' }],
     }),
     // Alphabetical order with multiple groups
     test({
@@ -391,23 +399,23 @@ ruleTester.run('order', rule, {
         import path from 'path';
         import async from 'async';
       `,
-      options: [{ 'sort-order': 'alphabetical' }],
+      options: [{ 'sort': 'alphabetical' }],
     }),
     // Ignore alphabetical order
     test({
       code: `
         import path from 'path';
         import fs from 'fs';
       `,
-      options: [{ 'sort-order': 'ignore' }],
+      options: [{ 'sort': 'ignore' }],
     }),
     // Ignore alphabetical order across groups
     test({
       code: `
         import fs from 'fs';
         import async from 'async';
       `,
-      options: [{ 'sort-order': 'alphabetical' }],
+      options: [{ 'sort': 'alphabetical' }],
     }),
   ],
   invalid: [
@@ -799,7 +807,22 @@ ruleTester.run('order', rule, {
         import path from 'path';
         import fs from 'fs';
       `,
-      options: [{ 'sort-order': 'alphabetical' }],
+      options: [{ 'sort': 'alphabetical' }],
+      errors: [
+        {
+          ruleId: 'order',
+          message: '`fs` import should occur before import of `path`',
+        },
+      ],
+    }),
+    // Bad alphabetical order with duplicate import
+    test({
+      code: `
+        import fs from 'fs';
+        import path from 'path';
+        import fs from 'fs';
+      `,
+      options: [{ 'sort': 'alphabetical' }],
       errors: [
         {
           ruleId: 'order',
@@ -821,7 +844,7 @@ ruleTester.run('order', rule, {
             ['sibling'],
             ['parent', 'external'],
           ],
-          'sort-order': 'alphabetical',
+          'sort': 'alphabetical',
         }
       ],
       errors: [
@@ -838,7 +861,7 @@ ruleTester.run('order', rule, {
         import fs from 'fs';
         import path from 'path';
       `,
-      options: [{ 'sort-order': 'alphabetical' }],
+      options: [{ 'sort': 'alphabetical' }],
       errors: [
         {
           ruleId: 'order',
@@ -853,7 +876,7 @@ ruleTester.run('order', rule, {
         import path from 'path';
         import fs from 'fs';
       `,
-      options: [{ 'sort-order': 'alphabetical' }],
+      options: [{ 'sort': 'alphabetical' }],
       errors: [
         {
           ruleId: 'order',