Alphabetize imports within groups #1360

stropho · 2019-05-14T12:20:53Z

I'm not the real author of these commits. I just rebased the existing #1105 . I'm looking forward to use this new feature.

Closes #1105. Fixes #1406.

…n groups Fixes #1406. Fixes #389. Closes #629. Closes #1105. Closes #1360. Co-Authored-By: dannysindra <als478@cornell.edu> Co-Authored-By: Radim Svoboda <radim.svoboda@socialbakers.com> Co-Authored-By: Soma Lucz <luczsoma@gmail.com> Co-Authored-By: Randall Reed, Jr <randallreedjr@gmail.com> Co-Authored-By: Jordan Harband <ljharb@gmail.com>

coveralls · 2019-05-14T12:39:41Z

Coverage decreased (-0.1%) to 97.82% when pulling 86f6b42 on stropho:alphabetize into 2f85fbe on benmosher:master.

coveralls · 2019-05-14T12:39:41Z

Coverage decreased (-0.1%) to 96.338% when pulling aae9da7 on stropho:alphabetize into f12ae59 on benmosher:master.

stropho · 2019-05-14T14:43:18Z

Can anybody help? Is there any way to test incorrect configuration passed to the eslint rule? It seems like such errors are handled already by the RuleTester and not the rule itself, so I don't see i simple way to test it at the moment.

Or should we just delete the checks to make coveralls happy ?

ljharb · 2019-05-15T13:06:04Z

Please don’t open duplicate PRs; now i have to maintain both in sync, open, until both are merged together.

Instead, post a link to your commits on the original PR, and a maintainer will update it.

stropho · 2019-05-15T13:48:57Z

ok, got it

VincentLanglet · 2019-05-30T18:18:48Z

Is this ready to merge ? This feature will be great.

stropho · 2019-06-03T09:41:17Z

I believe it is ready. I created a fix based on the suggestion in #1105 . And now, probably some of the maintainers have to say "it's ok" and merge it.

syashenkov · 2019-06-05T09:43:31Z

I think example still needs correction:

import assert from 'assert';
import buffer from 'buffer';
import foo from 'foo';
import bar from 'bar';
import Baz from 'Baz';

It's unclear if imports should be sorted by module name or imported symbols. Also, it's uncertain what behaviour is expected using star imports (import * as) and symbol imports (import { A, B })

It would be best to make example with some real life imports like

import * as classnames from 'classnames';
import PropTypes from 'prop-types';
import React, { PureComponent } from 'react';
import { compose } from 'recompose';

syashenkov · 2019-06-05T11:44:02Z

Or a more clear example like

Pass:

import c from 'a';
import b from 'b';
import a from 'c';

Fail:

import a from 'c';
import b from 'b';
import c from 'a';

syashenkov · 2019-06-14T08:22:37Z

@ljharb Could you please look at this PR?

stropho · 2019-07-09T12:32:02Z

Fixed the example.
Rebased on master branch.
Hope everybody is happy now.

Travis CI failed but it has the same errors as master branch did https://travis-ci.org/benmosher/eslint-plugin-import/builds/553254212 . So I don't think there is anything I can do about it right now.

deb0ch · 2019-08-06T08:58:59Z

The tests don't seem to be failing on master anymore, is this mergeable soon before there are conflicts ?

stropho · 2019-08-06T11:40:48Z

@deb0ch should be mergable; thanks for the reminder, just rebased 🍾 🎉 🍾 🎉

luczsoma · 2019-08-23T09:02:17Z

@ljharb Could you look into merging this PR?

It is a long-wanted feature, and also would come with a few benefits, since it:

Fixes [import/order] sort order within groups #389
Closes Fix #389: Allow secondary alphabetical sorting #629
Closes Alphabetize imports within groups #1105

Thank you.

syabro · 2019-08-23T09:12:06Z

src/rules/order.js

+    groupedByRanks[groupRank].sort(function(importA, importB) {
+      return ignoreCase
+        ? importA.localeCompare(importB)
+        : (importA < importB ? -1 : (importA === importB ? 0 : 1))


Scary. Could it be replaced with if... return?

Btw it is also incorrect I think. The default sensitvity option is variant in localeCompare for the default sorting usage (applies in the above snippet), which does not ignore casing.

I don't want to meddle in this PR, but I suggest the following:

groupedByRanks[groupRank].sort(function(importA, importB) { return importA.localeCompare(importB, undefined, { sensitivity: ignoreCase ? 'accent' : 'case' }) })

luczsoma · 2019-08-23T12:57:49Z

@stropho, since I have no write access to this, in order to get this PR finally merged before any further conflicts emerge, I would like to ask you to

either commit the following two minor changes, please,
or invite me as a contributor into stropho/eslint-plugin-import-1 so I am able to commit them.

In src/rules/order.js, change this:

groupedByRanks[groupRank].sort(function(importA, importB) {
  return ignoreCase
    ? importA.localeCompare(importB)
    : (importA < importB ? -1 : (importA === importB ? 0 : 1))
})

to this:

groupedByRanks[groupRank].sort(function(importA, importB) {
  return importA.localeCompare(importB, undefined, { 
    sensitivity: ignoreCase ? 'accent' : 'case' 
  })
})

Add the following to CHANGELOG.md, into the Unreleased section:

- [`order`]: Add support for alphabetical sorting within import groups ([#1360], thanks [@duncanbeevers] & [@stropho])

Thank you!

stropho · 2019-08-26T09:36:48Z

@luczsoma I haven't really investigated the code much, just wanted to help to merge it.... anyway, I just tried to fix it myself since you've told me exactly what to do :-D About 4 tests fail now. That is ok but it seems to be nearly impossible for me to find which tests are failing.
I added you as collaborator... Feel free to add changes. I could find some spare time at the end of the week.

luczsoma · 2019-08-27T12:43:11Z

Thank you @stropho!

As I tried to extend the functionality with configurable numeric sorting (1 < 2 < 10 vs 1 < 10 < 2) and configurable casing options (a-z < A-Z vs a-z > A-Z), I found out that localeCompare doesn't really work as common sense expects, see https://stackoverflow.com/questions/57671159. For the sake of simplicity, I removed localeCompare.

I also fixed @syabro's concerns.

luczsoma · 2019-08-27T12:44:47Z

I think this should be mergeable now.

syabro · 2019-08-27T13:23:17Z

As I tried to extend the functionality with configurable numeric sorting (1 < 2 < 10 vs 1 < 10 < 2) and configurable casing options (a-z < A-Z vs a-z > A-Z)

I bet it could be done later :) We badly need a basic sort

luczsoma · 2019-08-27T13:34:41Z

As I tried to extend the functionality with configurable numeric sorting (1 < 2 < 10 vs 1 < 10 < 2) and configurable casing options (a-z < A-Z vs a-z > A-Z)

I bet it could be done later :) We badly need a basic sort

Planning to do it some time later. :)

stropho · 2019-08-28T11:33:04Z

thx @luczsoma - I'm not a fan of rewriting code to make it bigger but as long as it is more readable and everybody's happy about it.. why not...

What got my attention though is that the ignoreCase probably does not work as it was intended.
My understanding when ignoreCase=true: it doesn't matter if a is before or after A
My understanding of how it works now: sorting treats a that it equals A but the tested code needs to be equal to the result of sorting :-/
So I changed the tests a bit to actually test the ignoreCase option and had to comment 2 of them because it seems they are not fixable with the current implementation. So what now ?

easier choice - remove ignoreCase option. Why would anyone create files with capital or non-ascii letters ? I understand that this is strongly opinionated. But if anyone will ever want such feature, it could be added later. As mentioned before:

We badly need a basic sort

fix the ignoreCase case. This does not quite fit my schedule. But maybe someone else can try ?

Let's vote!!!
👍 for option 1.
🚀 for option 2.

stropho · 2019-08-28T12:58:50Z

alright, ignoreCase option is removed. Less options --> smaller chance something is not working 😉

stropho · 2019-09-13T09:51:25Z

@ljharb any news? any suggestions to get it merged ?

aunnk · 2019-09-19T04:19:15Z

any updates on this? really want it to be merged.

src/rules/order.js

ljharb · 2019-09-19T05:02:00Z

tests/src/rules/order.js

@@ -335,10 +335,10 @@ ruleTester.run('order', rule, {
    // Option newlines-between: 'always' with multiline imports #3
    test({
      code: `
-        import foo
+        import a


why does this need to be changed? can this test be reverted?

I think the intention of this commit (which is not my own) was to show how the alphabetical sorting in a clearer way, with minimal examples. I would not revert that.

If your comment is concerned about only this particular test case (which tests multiline, not ordering), I will revert that part to its original state.

Yes, my comment is because for it not to be a breaking change, no existing test cases should be altered.

ljharb · 2019-09-19T05:02:24Z

(additionally, please rebase this on latest master)

luczsoma · 2019-09-19T12:22:11Z

Requested changes are done, rebased onto master.

luczsoma · 2019-09-24T08:15:14Z

@ljharb Is there anything else we can do to get this merged? I cannot dismiss the change request (although everything requested is done), only you (or the author of this PR, @stropho) can.

nmorel · 2019-10-14T10:03:29Z

I tried the PR and it breaks the newlines-between rules.
With always, I have an empty line between all my imports and not just groups.
From what I see, newlines-between uses rank to determine empty lines to add and alphabetize modifies the ranks to order.

nmorel · 2019-10-14T10:11:01Z

Putting the newlines test first seems to fix it :

if (newlinesBetweenImports !== 'ignore') {
  makeNewlinesBetweenReport(context, imported, newlinesBetweenImports)
}

if (alphabetize.order !== 'ignore') {
  mutateRanksToAlphabetize(imported, alphabetize.order)
}

makeOutOfOrderReport(context, imported)

ljharb

Because of the autofixer, if this ends up breaking side effecty modules by sorting anything across bindingless imports (which should never themselves be sorted), then the autofix for this entire rule will be promptly disabled in a patch.

Please do feel motivated to quickly open a followup PR with test cases that cover those cases if you're interested in ensuring this rule remains autofixable.

…n groups Fixes import-js#1406. Fixes import-js#389. Closes import-js#629. Closes import-js#1105. Closes import-js#1360. Co-Authored-By: dannysindra <als478@cornell.edu> Co-Authored-By: Radim Svoboda <radim.svoboda@socialbakers.com> Co-Authored-By: Soma Lucz <luczsoma@gmail.com> Co-Authored-By: Randall Reed, Jr <randallreedjr@gmail.com> Co-Authored-By: Jordan Harband <ljharb@gmail.com>

Manually alphabetize import groups and lines pending the merging of import-js/eslint-plugin-import#1360, using WebStorm's Optimize imports

ljharb added the import/export ordering label May 15, 2019

stropho closed this May 15, 2019

ljharb reopened this May 15, 2019

stropho mentioned this pull request Jun 4, 2019

Alphabetize imports within groups #1105

Merged

Jaguar0625 mentioned this pull request Jul 18, 2019

upgrade all packages using yarn upgrade --latest symbol/catapult-rest#61

Closed

syabro reviewed Aug 23, 2019

View reviewed changes

jbalsas mentioned this pull request Aug 24, 2019

Sort imports liferay/eslint-config-liferay#70

Closed

heygrady mentioned this pull request Sep 5, 2019

Group for first party packages lydell/eslint-plugin-simple-import-sort#18

Closed

ljharb requested changes Sep 19, 2019

View reviewed changes

stropho requested a review from ljharb September 24, 2019 09:03

This was referenced Oct 14, 2019

Lint imports taniarascia/takenote#30

Closed

Set up linting taniarascia/takenote#29

Merged

This comment has been minimized.

Sign in to view

ljharb mentioned this pull request Nov 26, 2019

[import/order] sort order within groups #389

Closed

This comment has been minimized.

Sign in to view

ljharb approved these changes Dec 7, 2019

View reviewed changes

ljharb closed this in 8224e51 Dec 7, 2019

ljharb merged commit 8224e51 into import-js:master Dec 7, 2019

Jaguar0625 mentioned this pull request Dec 8, 2019

enable eslint alphabetization rule symbol/catapult-rest#224

Closed

AamuLumi mentioned this pull request Dec 9, 2019

[import/order] Fix alphabetize bug with newlines-between #1562

Merged

ljharb mentioned this pull request Dec 9, 2019

import/order: alphabetize doesn't work correctly with group & newlines-between #1561

Closed

jaynilson added a commit to jaynilson/react_takenote_Persian that referenced this pull request Sep 10, 2024

Alphabetize imports

a9a9152

Manually alphabetize import groups and lines pending the merging of import-js/eslint-plugin-import#1360, using WebStorm's Optimize imports

Alphabetize imports within groups #1360

Alphabetize imports within groups #1360

Conversation

stropho commented May 14, 2019 • edited by ljharb Loading

coveralls commented May 14, 2019

coveralls commented May 14, 2019 • edited Loading

stropho commented May 14, 2019

ljharb commented May 15, 2019 • edited Loading

stropho commented May 15, 2019

VincentLanglet commented May 30, 2019

stropho commented Jun 3, 2019

syashenkov commented Jun 5, 2019 • edited Loading

syashenkov commented Jun 5, 2019

syashenkov commented Jun 14, 2019

stropho commented Jul 9, 2019

deb0ch commented Aug 6, 2019

stropho commented Aug 6, 2019

luczsoma commented Aug 23, 2019

syabro Aug 23, 2019

Choose a reason for hiding this comment

luczsoma Aug 23, 2019

Choose a reason for hiding this comment

luczsoma commented Aug 23, 2019

stropho commented Aug 26, 2019

luczsoma commented Aug 27, 2019

luczsoma commented Aug 27, 2019

syabro commented Aug 27, 2019

luczsoma commented Aug 27, 2019

stropho commented Aug 28, 2019

stropho commented Aug 28, 2019

stropho commented Sep 13, 2019

aunnk commented Sep 19, 2019

ljharb Sep 19, 2019

Choose a reason for hiding this comment

luczsoma Sep 19, 2019

Choose a reason for hiding this comment

luczsoma Sep 19, 2019

Choose a reason for hiding this comment

ljharb Dec 7, 2019

Choose a reason for hiding this comment

ljharb commented Sep 19, 2019

luczsoma commented Sep 19, 2019

luczsoma commented Sep 24, 2019

nmorel commented Oct 14, 2019

nmorel commented Oct 14, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ljharb left a comment

Choose a reason for hiding this comment

stropho commented May 14, 2019 •

edited by ljharb

Loading

coveralls commented May 14, 2019 •

edited

Loading

ljharb commented May 15, 2019 •

edited

Loading

syashenkov commented Jun 5, 2019 •

edited

Loading