Update: Allows for grouped sort-imports (fixes #12951) #12956

dmitov · 2020-02-23T12:15:44Z

Prerequisites checklist

I have read the contributing guidelines.
The team has reached consensus on the changes proposed in this pull request. If not, I understand that the evaluation process will begin with this pull request and won't be merged until the team has reached consensus.

What is the purpose of this pull request? (put an "X" next to an item)

What changes did you make? (Give an overview)

I've added a new option to the sort-imports rule, called ignoreBlankLines, which is true by default. When set to false, the alphabetical sort error will be surpassed if there is a blank line before the statement. Meaning that there could be grouped imports, like mentioned in the issue.

Is there anything you'd like reviewers to focus on?

Maybe the default to true option could be done better, but the tests are working nice, so I suppose it's fine.

jsf-clabot · 2020-02-23T12:15:46Z

All committers have signed the CLA.

yeonjuan · 2020-02-24T12:41:29Z

Hello, @dmitov Thanks for the proposal! (#12951)
And please note that we need time to get the consensus of ESlint members on the proposal. :) You can find more about the consensus process here.

dmitov · 2020-02-24T18:19:43Z

@yeonjuan Thanks for your response! I'm familiar with the consensus process and I hope this will pass it, or inspire someone else to implement it in the core.

aladdin-add · 2020-04-16T06:39:12Z

docs/rules/sort-imports.md

@@ -36,6 +36,7 @@ This rule accepts an object with its properties as
 * `ignoreCase` (default: `false`)
 * `ignoreDeclarationSort` (default: `false`)
 * `ignoreMemberSort` (default: `false`)
+* `ignoreBlankLines` (default: `true`)


this might be a breaking change, as we follow semantic-versioning-policy

I think it wouldn't be breaking, given the description in #12951, but this option's name is definitely confusing and we should change it.

it is a breaking change, as users have to update their config to keep the current behaviour.
am I misunderstanding something?

This will be more clear with allowSeparatedGroups name.

Per the original proposal, ignoreBlankLines: true was supposed to keep the current behavior, so it wouldn't be necessary to update configs.

I believe the logic for ignoreBlankLines was: blank lines should normally reset sorting, but by default this rule ignores them.

I see, thanks! ❤️

mdjermanovic · 2020-04-16T17:52:55Z

docs/rules/sort-imports.md

@@ -36,6 +36,7 @@ This rule accepts an object with its properties as
 * `ignoreCase` (default: `false`)
 * `ignoreDeclarationSort` (default: `false`)
 * `ignoreMemberSort` (default: `false`)
+* `ignoreBlankLines` (default: `true`)


We should rename this option to allowSeparatedGroups, default false.

(the same applies to the rest of documentation, and the code of course)

mdjermanovic · 2020-04-16T20:55:59Z

docs/rules/sort-imports.md

@@ -186,6 +200,45 @@ import {b, a, c} from 'foo.js'

 Default is `false`.

+### `ignoreBlankLines`
+
+When `true` the rule ignores the blank lines between the imports.


Along with the option's name, we should also change this description since this option doesn't check blank lines (#12951 comment)

mdjermanovic · 2020-04-16T20:57:33Z

docs/rules/sort-imports.md

+
+When `true` the rule ignores the blank lines between the imports.
+
+Examples of **incorrect** code for this rule with the default `{ "ignoreBlankLines": true }` option:


Can you please add an example with a statement between two imports, and with a comment between imports, both without any blank lines.

(same examples in incorrect and correct)

mdjermanovic · 2020-04-16T21:04:05Z

lib/rules/sort-imports.js

+         * @param {ASTNode} node2 the ImportDeclaration node.
+         * @returns {number} number of lines.
+         */
+        function getLinesBetweenMembers(node1, node2) {


Perhaps we should rename this function to something like getNumberOfLinesBetween (as in function's description), because it doesn't return lines?

mdjermanovic · 2020-04-16T21:28:29Z

lib/rules/sort-imports.js

+         * @param {ASTNode} node1 the ImportDeclaration node.
+         * @param {ASTNode} node2 the ImportDeclaration node.


This function assumes that node1 appears after node2 in the source code, so it would be nice to rename these params to make it clear. Maybe declaration and previousDeclaration ?

mdjermanovic · 2020-04-16T21:33:02Z

tests/lib/rules/sort-imports.js

+                "import b from 'foo.js';\n\n" +
+                "import a from 'bar.js';",
+            output: null,
+            errors: [expectedError]


It would be nice to have an invalid test where the option is explicitly set to its default value.

Can you please also add some tests with multiline declarations and some tests where both declarations are on the same line? This is to ensure that the code that counts lines and checks the result works well.

Also, tests with a comment between, and tests with a statement between instead of blank lines.

nzakas · 2020-06-11T18:39:46Z

It looks like this pull request has been abandoned, so I'm closing it.

Note: the issue that this pull request addresses is accepted (#12951), so anyone who wants to work on this change may do so and submit another pull request.

eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Feb 23, 2020

dmitov force-pushed the issue12951 branch from 96b1881 to 0fbc41c Compare February 23, 2020 12:18

Update: Allows for grouped sort-imports (fixes eslint#12951)

82e6d53

dmitov force-pushed the issue12951 branch from 0fbc41c to 82e6d53 Compare February 23, 2020 12:27

dmitov requested a review from g-plane February 23, 2020 12:53

dmitov removed the request for review from g-plane February 24, 2020 18:23

mdjermanovic linked an issue Apr 2, 2020 that may be closed by this pull request

[sort-imports] Allow the users to group their imports in a simple way #12951

Closed

mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Apr 2, 2020

aladdin-add self-requested a review April 15, 2020 06:13

aladdin-add requested changes Apr 16, 2020

View reviewed changes

mdjermanovic requested changes Apr 16, 2020

View reviewed changes

anikethsaha mentioned this pull request Apr 24, 2020

sort-imports not behaving as expected #13222

Closed

nzakas closed this Jun 11, 2020

eslint-deprecated bot locked and limited conversation to collaborators Dec 9, 2020

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Dec 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update: Allows for grouped sort-imports (fixes #12951) #12956

Update: Allows for grouped sort-imports (fixes #12951) #12956

dmitov commented Feb 23, 2020

jsf-clabot commented Feb 23, 2020 •

edited

yeonjuan commented Feb 24, 2020

dmitov commented Feb 24, 2020

aladdin-add Apr 16, 2020

mdjermanovic Apr 16, 2020

aladdin-add Apr 17, 2020

mdjermanovic Apr 18, 2020

aladdin-add Apr 18, 2020

mdjermanovic Apr 16, 2020

mdjermanovic Apr 16, 2020

mdjermanovic Apr 16, 2020 •

edited

mdjermanovic Apr 16, 2020

mdjermanovic Apr 16, 2020

mdjermanovic Apr 16, 2020

nzakas commented Jun 11, 2020


		When `true` the rule ignores the blank lines between the imports.

		Examples of incorrect code for this rule with the default `{ "ignoreBlankLines": true }` option:

		* @param {ASTNode} node1 the ImportDeclaration node.
		* @param {ASTNode} node2 the ImportDeclaration node.

Update: Allows for grouped sort-imports (fixes #12951) #12956

Update: Allows for grouped sort-imports (fixes #12951) #12956

Conversation

dmitov commented Feb 23, 2020

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

What changes did you make? (Give an overview)

Is there anything you'd like reviewers to focus on?

jsf-clabot commented Feb 23, 2020 • edited

yeonjuan commented Feb 24, 2020

dmitov commented Feb 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdjermanovic Apr 16, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nzakas commented Jun 11, 2020

jsf-clabot commented Feb 23, 2020 •

edited

mdjermanovic Apr 16, 2020 •

edited