[New] order: Add distinctGroup option

Fixes #2292.
import-js · Aug 14, 2022 · 998655b · 998655b
1 parent a8781f7
commit 998655b
Show file tree

Hide file tree

Showing 4 changed files with 268 additions and 14 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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({