From 8eeb7618735a19ccfe1d2f5bd483bfe7225312bf Mon Sep 17 00:00:00 2001 From: "Randall Reed, Jr" Date: Tue, 18 Oct 2016 21:41:11 -0400 Subject: [PATCH] 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 | 8 ++++---- src/rules/order.js | 42 +++++++++++++++++++++------------------- tests/src/rules/order.js | 39 +++++++++++++++++++++++++++++-------- 3 files changed, 57 insertions(+), 32 deletions(-) diff --git a/docs/rules/order.md b/docs/rules/order.md index 8fb239555..e716851f6 100644 --- a/docs/rules/order.md +++ b/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,7 +162,7 @@ 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 './'; @@ -170,7 +170,7 @@ 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 './'; diff --git a/src/rules/order.js b/src/rules/order.js index 414b6ae40..0f6215564 100644 --- a/src/rules/order.js +++ b/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) diff --git a/tests/src/rules/order.js b/tests/src/rules/order.js index 5bd4298d0..b1217677c 100644 --- a/tests/src/rules/order.js +++ b/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,7 +399,7 @@ ruleTester.run('order', rule, { import path from 'path'; import async from 'async'; `, - options: [{ 'sort-order': 'alphabetical' }], + options: [{ 'sort': 'alphabetical' }], }), // Ignore alphabetical order test({ @@ -399,7 +407,7 @@ ruleTester.run('order', rule, { import path from 'path'; import fs from 'fs'; `, - options: [{ 'sort-order': 'ignore' }], + options: [{ 'sort': 'ignore' }], }), // Ignore alphabetical order across groups test({ @@ -407,7 +415,7 @@ ruleTester.run('order', rule, { 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',