Skip to content

Commit

Permalink
Allow secondary alphabetical sort of imports
Browse files Browse the repository at this point in the history
* Define comparators that take into account rank and name
  • Loading branch information
randallreedjr committed Dec 5, 2016
1 parent bfdc2bb commit c0dea5e
Show file tree
Hide file tree
Showing 4 changed files with 214 additions and 29 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel
- Fix crash when using [`newline-after-import`] with decorators ([#592])
- Properly report [`newline-after-import`] when next line is a decorator
- Fixed documentation for the default values for the [`order`] rule ([#601])
- [`order`] allows secondary alphabetical sort ([#389])

## [2.0.1] - 2016-10-06
### Fixed
Expand Down
36 changes: 36 additions & 0 deletions docs/rules/order.md
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,42 @@ 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.

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"}] */
/* eslint import/order: ["error"] */
import path from 'path';
import fs from 'fs';
import index from './';
import sibling from './foo';
```

## Related

- [`import/external-module-folders`] setting
Expand Down
76 changes: 48 additions & 28 deletions src/rules/order.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,53 +7,54 @@ const defaultGroups = ['builtin', 'external', 'parent', 'sibling', 'index']

// REPORTING

function reverse(array) {
return array.map(function (v) {
return {
name: v.name,
rank: -v.rank,
node: v.node,
}
}).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
})
}

function reportOutOfOrder(context, imported, outOfOrder, order) {
outOfOrder.forEach(function (imp) {
const found = imported.find(function hasHigherRank(importedItem) {
return importedItem.rank > imp.rank
})
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)
if (!outOfOrder.length) {
function makeOutOfOrderReport(context, imported, forwardSortComparator, reverseSortComparator) {
const outOfOrder = findOutOfOrder(imported, reverseSortComparator)
if (outOfOrder.length === 0) {
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
Expand Down Expand Up @@ -158,6 +159,9 @@ module.exports = {
'newlines-between': {
enum: [ 'ignore', 'always', 'never' ],
},
'sort': {
enum: [ 'ignore', 'alphabetical' ],
},
},
additionalProperties: false,
},
Expand Down Expand Up @@ -189,6 +193,20 @@ module.exports = {
level--
}

function determineComparators(alphabetize) {
let forwardSortComparator, reverseSortComparator
if (alphabetize) {
forwardSortComparator = (a, b) => a.rank > b.rank ||
(a.rank === b.rank && a.name > b.name)
reverseSortComparator = (a, b) => a.rank < b.rank ||
(a.rank === b.rank && a.name < b.name)
} else {
forwardSortComparator = (a, b) => a.rank > b.rank
reverseSortComparator = (a, b) => a.rank < b.rank
}
return [forwardSortComparator, reverseSortComparator]
}

return {
ImportDeclaration: function handleImports(node) {
if (node.specifiers.length) { // Ignoring unassigned imports
Expand All @@ -204,7 +222,9 @@ module.exports = {
registerNode(context, node, name, 'require', ranks, imported)
},
'Program:exit': function reportAndReset() {
makeOutOfOrderReport(context, imported)
const alphabetize = (options['sort'] === 'alphabetical')
let [forwardSortComparator, reverseSortComparator] = determineComparators(alphabetize)
makeOutOfOrderReport(context, imported, forwardSortComparator, reverseSortComparator)

if (newlinesBetweenImports !== 'ignore') {
makeNewlinesBetweenReport(context, imported, newlinesBetweenImports)
Expand Down
130 changes: 129 additions & 1 deletion tests/src/rules/order.js
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,47 @@ ruleTester.run('order', rule, {
`,
options: [{ 'newlines-between': 'always' }]
}),
// Alphabetical order
test({
code: `
import fs from 'fs';
import path from 'path';
`,
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({
code: `
import fs from 'fs';
import path from 'path';
import async from 'async';
`,
options: [{ 'sort': 'alphabetical' }],
}),
// Ignore alphabetical order
test({
code: `
import path from 'path';
import fs from 'fs';
`,
options: [{ 'sort': 'ignore' }],
}),
// Ignore alphabetical order across groups
test({
code: `
import fs from 'fs';
import async from 'async';
`,
options: [{ 'sort': 'alphabetical' }],
}),
],
invalid: [
// builtin before external module (require)
Expand Down Expand Up @@ -456,7 +497,7 @@ ruleTester.run('order', rule, {
message: '`async` import should occur before import of `./sibling`',
}, {
ruleId: 'order',
message: '`fs` import should occur before import of `./sibling`',
message: '`fs` import should occur before import of `async`',
}],
}),
// Uses 'after' wording if it creates less errors
Expand Down Expand Up @@ -760,5 +801,92 @@ ruleTester.run('order', rule, {
},
],
}),
// Bad alphabetical order
test({
code: `
import path from 'path';
import fs from 'fs';
`,
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',
message: '`fs` import should occur before import of `path`',
},
],
}),
// Bad alphabetical order reverse
test({
code: `
import path from 'path';
import index from './';
import fs from 'fs';
`,
options: [
{
groups: [
['builtin', 'index'],
['sibling'],
['parent', 'external'],
],
'sort': 'alphabetical',
}
],
errors: [
{
ruleId: 'order',
message: '`path` import should occur after import of `fs`',
},
],
}),
// Good alphabetical order with incorrect group order
test({
code: `
import async from 'async'
import fs from 'fs';
import path from 'path';
`,
options: [{ 'sort': 'alphabetical' }],
errors: [
{
ruleId: 'order',
message: '`async` import should occur after import of `path`',
},
],
}),
// Bad alphabetical order with incorrect group order
test({
code: `
import async from 'async'
import path from 'path';
import fs from 'fs';
`,
options: [{ 'sort': 'alphabetical' }],
errors: [
{
ruleId: 'order',
message: '`path` import should occur before import of `async`',
},
{
ruleId: 'order',
message: '`fs` import should occur before import of `path`',
},
],
}),
],
})

0 comments on commit c0dea5e

Please sign in to comment.