From 998655b1438c9718403ae876163b739c57f658ca Mon Sep 17 00:00:00 2001 From: Edwin Kofler Date: Sat, 13 Aug 2022 17:34:22 -0700 Subject: [PATCH] [New] `order`: Add `distinctGroup` option Fixes #2292. --- CHANGELOG.md | 2 + docs/rules/order.md | 25 +++++ src/rules/order.js | 44 +++++--- tests/src/rules/order.js | 211 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 268 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d54c4266b..27c59ac03 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ This change log adheres to standards from [Keep a CHANGELOG](https://keepachange - [`no-cycle`]: add `allowUnsafeDynamicCyclicDependency` option ([#2387], thanks [@GerkinDev]) - [`no-restricted-paths`]: support arrays for `from` and `target` options ([#2466], thanks [@AdriAt360]) - [`no-anonymous-default-export`]: add `allowNew` option ([#2505], thanks [@DamienCassou]) +- [`order`]: Add `distinctGroup` option ([#2395], thanks [@hyperupcall]) ### Fixed - [`order`]: move nested imports closer to main import entry ([#2396], thanks [@pri1311]) @@ -1014,6 +1015,7 @@ for info on changes for earlier releases. [#2411]: https://github.com/import-js/eslint-plugin-import/pull/2411 [#2399]: https://github.com/import-js/eslint-plugin-import/pull/2399 [#2396]: https://github.com/import-js/eslint-plugin-import/pull/2396 +[#2395]: https://github.com/import-js/eslint-plugin-import/pull/2395 [#2393]: https://github.com/import-js/eslint-plugin-import/pull/2393 [#2388]: https://github.com/import-js/eslint-plugin-import/pull/2388 [#2387]: https://github.com/import-js/eslint-plugin-import/pull/2387 diff --git a/docs/rules/order.md b/docs/rules/order.md index 8eead0935..53faff153 100644 --- a/docs/rules/order.md +++ b/docs/rules/order.md @@ -128,6 +128,31 @@ Properties of the objects } ``` +### `distinctGroup: [boolean]`: + +This changes how `pathGroups[].position` affects grouping. The property is most useful when `newlines-between` is set to `always` and at least 1 `pathGroups` entry has a `position` property set. + +By default, in the context of a particular `pathGroup` entry, when setting `position`, a new "group" will silently be created. That is, even if the `group` is specified, a newline will still separate imports that match that `pattern` with the rest of the group (assuming `newlines-between` is `always`). This is undesirable if your intentions are to use `position` to position _within_ the group (and not create a new one). Override this behavior by setting `distinctGroup` to `false`; this will keep imports within the same group as intended. + +Note that currently, `distinctGroup` defaults to `true`. However, in a later update, the default will change to `false` + +Example: +```json +{ + "import/order": ["error", { + "newlines-between": "always", + "pathGroups": [ + { + "pattern": "@app/**", + "group": "external", + "position": "after" + } + ], + "distinctGroup": false + }] +} +``` + ### `pathGroupsExcludedImportTypes: [array]`: This defines import types that are not handled by configured pathGroups. diff --git a/src/rules/order.js b/src/rules/order.js index 4fb178768..95311c0bc 100644 --- a/src/rules/order.js +++ b/src/rules/order.js @@ -493,7 +493,7 @@ function removeNewLineAfterImport(context, currentImport, previousImport) { return undefined; } -function makeNewlinesBetweenReport(context, imported, newlinesBetweenImports) { +function makeNewlinesBetweenReport(context, imported, newlinesBetweenImports, distinctGroup) { const getNumberOfEmptyLinesBetween = (currentImport, previousImport) => { const linesBetweenImports = context.getSourceCode().lines.slice( previousImport.node.loc.end.line, @@ -502,27 +502,34 @@ function makeNewlinesBetweenReport(context, imported, newlinesBetweenImports) { return linesBetweenImports.filter((line) => !line.trim().length).length; }; + const getIsStartOfDistinctGroup = (currentImport, previousImport) => { + return currentImport.rank - 1 >= previousImport.rank; + }; let previousImport = imported[0]; imported.slice(1).forEach(function (currentImport) { const emptyLinesBetween = getNumberOfEmptyLinesBetween(currentImport, previousImport); + const isStartOfDistinctGroup = getIsStartOfDistinctGroup(currentImport, previousImport); if (newlinesBetweenImports === 'always' || newlinesBetweenImports === 'always-and-inside-groups') { if (currentImport.rank !== previousImport.rank && emptyLinesBetween === 0) { - context.report({ - node: previousImport.node, - message: 'There should be at least one empty line between import groups', - fix: fixNewLineAfterImport(context, previousImport), - }); - } else if (currentImport.rank === previousImport.rank - && emptyLinesBetween > 0 + if (distinctGroup || (!distinctGroup && isStartOfDistinctGroup)) { + context.report({ + node: previousImport.node, + message: 'There should be at least one empty line between import groups', + fix: fixNewLineAfterImport(context, previousImport), + }); + } + } else if (emptyLinesBetween > 0 && newlinesBetweenImports !== 'always-and-inside-groups') { - context.report({ - node: previousImport.node, - message: 'There should be no empty line within import group', - fix: removeNewLineAfterImport(context, currentImport, previousImport), - }); + if ((distinctGroup && currentImport.rank === previousImport.rank) || (!distinctGroup && !isStartOfDistinctGroup)) { + context.report({ + node: previousImport.node, + message: 'There should be no empty line within import group', + fix: removeNewLineAfterImport(context, currentImport, previousImport), + }); + } } } else if (emptyLinesBetween > 0) { context.report({ @@ -544,6 +551,9 @@ function getAlphabetizeConfig(options) { return { order, caseInsensitive }; } +// TODO, semver-major: Change the default of "distinctGroup" from true to false +const defaultDistinctGroup = true; + module.exports = { meta: { type: 'suggestion', @@ -562,6 +572,10 @@ module.exports = { pathGroupsExcludedImportTypes: { type: 'array', }, + distinctGroup: { + type: 'boolean', + default: defaultDistinctGroup, + }, pathGroups: { type: 'array', items: { @@ -582,6 +596,7 @@ module.exports = { enum: ['after', 'before'], }, }, + additionalProperties: false, required: ['pattern', 'group'], }, }, @@ -622,6 +637,7 @@ module.exports = { const newlinesBetweenImports = options['newlines-between'] || 'ignore'; const pathGroupsExcludedImportTypes = new Set(options['pathGroupsExcludedImportTypes'] || ['builtin', 'external', 'object']); const alphabetize = getAlphabetizeConfig(options); + const distinctGroup = options.distinctGroup == null ? defaultDistinctGroup : !!options.distinctGroup; let ranks; try { @@ -724,7 +740,7 @@ module.exports = { 'Program:exit': function reportAndReset() { importMap.forEach((imported) => { if (newlinesBetweenImports !== 'ignore') { - makeNewlinesBetweenReport(context, imported, newlinesBetweenImports); + makeNewlinesBetweenReport(context, imported, newlinesBetweenImports, distinctGroup); } if (alphabetize.order !== 'ignore') { diff --git a/tests/src/rules/order.js b/tests/src/rules/order.js index ed4879ac4..75c19d415 100644 --- a/tests/src/rules/order.js +++ b/tests/src/rules/order.js @@ -925,6 +925,161 @@ ruleTester.run('order', rule, { }, }, }), + // Option pathGroup[].distinctGroup: 'true' does not prevent 'position' properties from affecting the visible grouping + test({ + code: ` + import A from 'a'; + + import C from 'c'; + + import B from 'b'; + `, + options: [ + { + 'newlines-between': 'always', + 'distinctGroup': true, + 'pathGroupsExcludedImportTypes': [], + 'pathGroups': [ + { + 'pattern': 'a', + 'group': 'external', + 'position': 'before', + }, + { + 'pattern': 'b', + 'group': 'external', + 'position': 'after', + }, + ], + }, + ], + }), + // Option pathGroup[].distinctGroup: 'false' should prevent 'position' properties from affecting the visible grouping + test({ + code: ` + import A from 'a'; + import C from 'c'; + import B from 'b'; + `, + options: [ + { + 'newlines-between': 'always', + 'distinctGroup': false, + 'pathGroupsExcludedImportTypes': [], + 'pathGroups': [ + { + 'pattern': 'a', + 'group': 'external', + 'position': 'before', + }, + { + 'pattern': 'b', + 'group': 'external', + 'position': 'after', + }, + ], + }, + ], + }), + // Option pathGroup[].distinctGroup: 'false' should prevent 'position' properties from affecting the visible grouping 2 + test({ + code: ` + import A from 'a'; + + import b from './b'; + import B from './B'; + `, + options: [ + { + 'newlines-between': 'always', + 'distinctGroup': false, + 'pathGroupsExcludedImportTypes': [], + 'pathGroups': [ + { + 'pattern': 'a', + 'group': 'external', + }, + { + 'pattern': 'b', + 'group': 'internal', + 'position': 'before', + }, + ], + }, + ], + }), + // Option pathGroup[].distinctGroup: 'false' should prevent 'position' properties from affecting the visible grouping 3 + test({ + code: ` + import A from "baz"; + import B from "Bar"; + import C from "Foo"; + + import D from ".."; + import E from "../"; + import F from "../baz"; + import G from "../Bar"; + import H from "../Foo"; + + import I from "."; + import J from "./baz"; + import K from "./Bar"; + import L from "./Foo"; + `, + options: [ + { + 'alphabetize': { + 'caseInsensitive': false, + 'order': 'asc', + }, + 'newlines-between': 'always', + 'groups': [ + ['builtin', 'external', 'internal', 'unknown', 'object', 'type'], + 'parent', + ['sibling', 'index'], + ], + 'distinctGroup': false, + 'pathGroupsExcludedImportTypes': [], + 'pathGroups': [ + { + 'pattern': './', + 'group': 'sibling', + 'position': 'before', + }, + { + 'pattern': '.', + 'group': 'sibling', + 'position': 'before', + }, + { + 'pattern': '..', + 'group': 'parent', + 'position': 'before', + }, + { + 'pattern': '../', + 'group': 'parent', + 'position': 'before', + }, + { + 'pattern': '[a-z]*', + 'group': 'external', + 'position': 'before', + }, + { + 'pattern': '../[a-z]*', + 'group': 'parent', + 'position': 'before', + }, + { + 'pattern': './[a-z]*', + 'group': 'sibling', + 'position': 'before', + }, + ], + }, + ], + }), ], invalid: [ // builtin before external module (require) @@ -2439,6 +2594,62 @@ ruleTester.run('order', rule, { message: '`..` import should occur before import of `../a`', }], }), + // Option pathGroup[].distinctGroup: 'false' should error when newlines are incorrect 2 + test({ + code: ` + import A from 'a'; + import C from './c'; + `, + output: ` + import A from 'a'; + + import C from './c'; + `, + options: [ + { + 'newlines-between': 'always', + 'distinctGroup': false, + 'pathGroupsExcludedImportTypes': [], + }, + ], + errors: [{ + message: 'There should be at least one empty line between import groups', + }], + }), + // Option pathGroup[].distinctGroup: 'false' should error when newlines are incorrect 2 + test({ + code: ` + import A from 'a'; + + import C from 'c'; + `, + output: ` + import A from 'a'; + import C from 'c'; + `, + options: [ + { + 'newlines-between': 'always', + 'distinctGroup': false, + 'pathGroupsExcludedImportTypes': [], + 'pathGroups': [ + { + 'pattern': 'a', + 'group': 'external', + 'position': 'before', + }, + { + 'pattern': 'c', + 'group': 'external', + 'position': 'after', + }, + ], + }, + ], + errors: [{ + message: 'There should be no empty line within import group', + }], + }), // Alphabetize with require ...semver.satisfies(eslintPkg.version, '< 3.0.0') ? [] : [ test({