From 6a4156d9862c3abcc05925bf816592b63a96e549 Mon Sep 17 00:00:00 2001 From: "Randall Reed, Jr" Date: Mon, 13 Feb 2017 17:46:58 -0500 Subject: [PATCH] [Fix] `order`: allow secondary alphabetical sorting Fixes #389. --- CHANGELOG.md | 1 + docs/rules/order.md | 35 ++++++++++++++++++++ src/rules/order.js | 70 +++++++++++++++++++++++++++++----------- tests/src/rules/order.js | 68 +++++++++++++++++++------------------- 4 files changed, 121 insertions(+), 53 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d384d5c390..bb906f90de 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel ### Added - Add ESLint 5 support ([#1122], thanks [@ai] and [@ljharb]) - Add [`no-relative-parent-imports`] rule: disallow relative imports from parent directories ([#1093], thanks [@chrislloyd]) +- [`order`] allows secondary alphabetical sort ([#389]) ### Fixed - `namespace` rule: ensure it works in eslint 5/ecmaVersion 2018 (thanks [@ljharb]) diff --git a/docs/rules/order.md b/docs/rules/order.md index 45bde6acc1..0449bca773 100644 --- a/docs/rules/order.md +++ b/docs/rules/order.md @@ -164,6 +164,41 @@ import index from './'; import sibling from './foo'; ``` +### `sort: [ignore|alphabetical]`: + +Enforces alphabetical sorting within import groups: + +- If set to `ignore`, no errors related to order within import groups will be reported (default). +- If set to `alphabetical`, imports within a group must be alphabetized. Imports across groups will not be compared. Alphabetical sort ignores capitalization. + +With the default group setting, the following will be invalid: + +```js +/* eslint import/order: ["error", {"sort": "alphabetical"}] */ +import path from 'path'; +import fs from 'fs'; +import index from './'; +import sibling from './foo'; +``` + +while this will be valid: + +```js +/* 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": "ignore"}] */ +import path from 'path'; +import fs from 'fs'; +import index from './'; +import sibling from './foo'; +``` + ## Related - [`import/external-module-folders`] setting diff --git a/src/rules/order.js b/src/rules/order.js index 81babd7fde..790104ed12 100644 --- a/src/rules/order.js +++ b/src/rules/order.js @@ -8,6 +8,7 @@ const defaultGroups = ['builtin', 'external', 'parent', 'sibling', 'index'] // REPORTING AND FIXING + function reverse(array) { return array.map(function (v) { return { @@ -72,14 +73,14 @@ function takeTokensBeforeWhile(sourceCode, node, condition) { return result.reverse() } -function findOutOfOrder(imported) { +function findOutOfOrder(imported, comparator) { if (imported.length === 0) { return [] } let maxSeenRankNode = imported[0] return imported.filter(function (importedModule) { - const res = importedModule.rank < maxSeenRankNode.rank - if (maxSeenRankNode.rank < importedModule.rank) { + const res = comparator(importedModule, maxSeenRankNode) + if (comparator(maxSeenRankNode, importedModule)) { maxSeenRankNode = importedModule } return res @@ -215,32 +216,58 @@ function fixOutOfOrder(context, firstNode, secondNode, order) { } } -function reportOutOfOrder(context, imported, outOfOrder, order) { - outOfOrder.forEach(function (imp) { - const found = imported.find(function hasHigherRank(importedItem) { - return importedItem.rank > imp.rank - }) - fixOutOfOrder(context, found, imp, order) - }) +function reportOutOfOrder(context, sortedImports, outOfOrder, order, comparator) { + // Pass in imports pre-sorted to ensure `found` is correct position + for (let imp of outOfOrder) { + const found = sortedImports.find(importedItem => comparator(importedItem, imp)) + + context.report(imp.node, '`' + imp.name + '` import should occur ' + order + + ' import of `' + found.name + '`') + } } -function makeOutOfOrderReport(context, imported) { - const outOfOrder = findOutOfOrder(imported) +function makeOutOfOrderReport(context, imported, forwardSortComparator, reverseSortComparator) { + const outOfOrder = findOutOfOrder(imported, reverseSortComparator) if (!outOfOrder.length) { return } // There are things to report. Try to minimize the number of reported errors. - const reversedImported = reverse(imported) - const reversedOrder = findOutOfOrder(reversedImported) + const reversedImported = [...imported].reverse() + const reversedOrder = findOutOfOrder(reversedImported, forwardSortComparator) + const sortedImports = [...imported].sort(forwardSortComparator) if (reversedOrder.length < outOfOrder.length) { - reportOutOfOrder(context, reversedImported, reversedOrder, 'after') - return + reportOutOfOrder(context, + sortedImports.reverse(), + reversedOrder, + 'after', + reverseSortComparator + ) + } else { + reportOutOfOrder(context, + sortedImports, + outOfOrder, + 'before', + forwardSortComparator + ) } - reportOutOfOrder(context, imported, outOfOrder, 'before') } // DETECTING +function determineComparators(alphabetize) { + let forwardSortComparator, reverseSortComparator + if (alphabetize) { + forwardSortComparator = (a, b) => a.rank > b.rank || + (a.rank === b.rank && (a.name.toLowerCase() > b.name.toLowerCase())) + reverseSortComparator = (a, b) => a.rank < b.rank || + (a.rank === b.rank && (a.name.toLowerCase() < b.name.toLowerCase())) + } else { + forwardSortComparator = (a, b) => a.rank > b.rank + reverseSortComparator = (a, b) => a.rank < b.rank + } + return [forwardSortComparator, reverseSortComparator] +} + function computeRank(context, ranks, name, type) { return ranks[importType(name, context)] + (type === 'import' ? 0 : 100) @@ -382,13 +409,16 @@ module.exports = { 'never', ], }, + 'sort': { + enum: [ 'ignore', 'alphabetical' ], + }, }, additionalProperties: false, }, ], }, - create: function importOrderRule (context) { + create: function importOrderRule(context) { const options = context.options[0] || {} const newlinesBetweenImports = options['newlines-between'] || 'ignore' let ranks @@ -428,7 +458,9 @@ module.exports = { registerNode(context, node, name, 'require', ranks, imported) }, 'Program:exit': function reportAndReset() { - makeOutOfOrderReport(context, imported) + const alphabetize = (options['sort'] === 'alphabetical') + const [forwardSortComparator, reverseSortComparator] = determineComparators(alphabetize) + makeOutOfOrderReport(context, imported, forwardSortComparator, reverseSortComparator) if (newlinesBetweenImports !== 'ignore') { makeNewlinesBetweenReport(context, imported, newlinesBetweenImports) diff --git a/tests/src/rules/order.js b/tests/src/rules/order.js index fb3b788448..59e9441075 100644 --- a/tests/src/rules/order.js +++ b/tests/src/rules/order.js @@ -3,7 +3,7 @@ import { test } from '../utils' import { RuleTester } from 'eslint' const ruleTester = new RuleTester() - , rule = require('rules/order') + , rule = require('rules/order') function withoutAutofixOutput(test) { return Object.assign({}, test, { output: test.code }) @@ -21,7 +21,7 @@ ruleTester.run('order', rule, { var relParent3 = require('../'); var sibling = require('./foo'); var index = require('./');`, - }), + }), // Default order using import test({ code: ` @@ -32,7 +32,7 @@ ruleTester.run('order', rule, { import relParent3 from '../'; import sibling, {foo3} from './foo'; import index from './';`, - }), + }), // Multiple module of the same rank next to each other test({ code: ` @@ -41,7 +41,7 @@ ruleTester.run('order', rule, { var path = require('path'); var _ = require('lodash'); var async = require('async');`, - }), + }), // Overriding order to be the reverse of the default order test({ code: ` @@ -136,9 +136,9 @@ ruleTester.run('order', rule, { var relParent1 = require('../foo'); `, options: [{groups: [ - ['builtin', 'index'], - ['sibling', 'parent', 'external'], - ]}], + ['builtin', 'index'], + ['sibling', 'parent', 'external'], + ]}], }), // Omitted types should implicitly be considered as the last type test({ @@ -147,10 +147,10 @@ ruleTester.run('order', rule, { var path = require('path'); `, options: [{groups: [ - 'index', - ['sibling', 'parent', 'external'], - // missing 'builtin' - ]}], + 'index', + ['sibling', 'parent', 'external'], + // missing 'builtin' + ]}], }), // Mixing require and import should have import up top test({ @@ -486,12 +486,12 @@ ruleTester.run('order', rule, { // fix order with windows end of lines test({ code: - `/* comment0 */ /* comment1 */ var async = require('async'); /* comment2 */` + `\r\n` + - `/* comment3 */ var fs = require('fs'); /* comment4 */` + `\r\n` + `/* comment0 */ /* comment1 */ var async = require('async'); /* comment2 */` + `\r\n` + + `/* comment3 */ var fs = require('fs'); /* comment4 */` + `\r\n` , output: - `/* comment3 */ var fs = require('fs'); /* comment4 */` + `\r\n` + - `/* comment0 */ /* comment1 */ var async = require('async'); /* comment2 */` + `\r\n` + `/* comment3 */ var fs = require('fs'); /* comment4 */` + `\r\n` + + `/* comment0 */ /* comment1 */ var async = require('async'); /* comment2 */` + `\r\n` , errors: [{ ruleId: 'order', @@ -740,9 +740,9 @@ ruleTester.run('order', rule, { var sibling = require('./foo'); `, options: [{groups: [ - ['builtin', 'index'], - ['sibling', 'parent', 'external'], - ]}], + ['builtin', 'index'], + ['sibling', 'parent', 'external'], + ]}], errors: [{ ruleId: 'order', message: '`path` import should occur before import of `./foo`', @@ -759,10 +759,10 @@ ruleTester.run('order', rule, { var path = require('path'); `, options: [{groups: [ - 'index', - ['sibling', 'parent', 'external', 'internal'], - // missing 'builtin' - ]}], + 'index', + ['sibling', 'parent', 'external', 'internal'], + // missing 'builtin' + ]}], errors: [{ ruleId: 'order', message: '`async` import should occur before import of `path`', @@ -776,9 +776,9 @@ ruleTester.run('order', rule, { var index = require('./'); `, options: [{groups: [ - 'index', - ['sibling', 'parent', 'UNKNOWN', 'internal'], - ]}], + 'index', + ['sibling', 'parent', 'UNKNOWN', 'internal'], + ]}], errors: [{ ruleId: 'order', message: 'Incorrect configuration of the rule: Unknown type `"UNKNOWN"`', @@ -791,9 +791,9 @@ ruleTester.run('order', rule, { var index = require('./'); `, options: [{groups: [ - 'index', - ['sibling', 'parent', ['builtin'], 'internal'], - ]}], + 'index', + ['sibling', 'parent', ['builtin'], 'internal'], + ]}], errors: [{ ruleId: 'order', message: 'Incorrect configuration of the rule: Unknown type `["builtin"]`', @@ -806,9 +806,9 @@ ruleTester.run('order', rule, { var index = require('./'); `, options: [{groups: [ - 'index', - ['sibling', 'parent', 2, 'internal'], - ]}], + 'index', + ['sibling', 'parent', 2, 'internal'], + ]}], errors: [{ ruleId: 'order', message: 'Incorrect configuration of the rule: Unknown type `2`', @@ -821,9 +821,9 @@ ruleTester.run('order', rule, { var index = require('./'); `, options: [{groups: [ - 'index', - ['sibling', 'parent', 'parent', 'internal'], - ]}], + 'index', + ['sibling', 'parent', 'parent', 'internal'], + ]}], errors: [{ ruleId: 'order', message: 'Incorrect configuration of the rule: `parent` is duplicated',