Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[New] order: add distinctGroup option #2395

Merged
merged 1 commit into from Aug 26, 2022
Merged

[New] order: add distinctGroup option #2395

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Expand Up @@ -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])
Expand Down Expand Up @@ -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
Expand Down
25 changes: 25 additions & 0 deletions docs/rules/order.md
View file
Open in desktop
Expand Up @@ -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.
Expand Down
44 changes: 30 additions & 14 deletions src/rules/order.js
View file
Open in desktop
Expand Up @@ -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,
Expand All @@ -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({
Expand All @@ -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',
Expand All @@ -562,6 +572,10 @@ module.exports = {
pathGroupsExcludedImportTypes: {
type: 'array',
},
distinctGroup: {
type: 'boolean',
default: defaultDistinctGroup,
},
pathGroups: {
type: 'array',
items: {
Expand All @@ -582,6 +596,7 @@ module.exports = {
enum: ['after', 'before'],
},
},
additionalProperties: false,
required: ['pattern', 'group'],
},
},
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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') {
Expand Down
211 changes: 211 additions & 0 deletions tests/src/rules/order.js
View file
Open in desktop
Expand Up @@ -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)
Expand Down Expand Up @@ -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({
Expand Down