-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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.
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
Fix #389: Allow secondary alphabetical sorting #629
Conversation
randallreedjr
commented
Oct 17, 2016
- After sorting imports by group, allow sorting alphabetically within a group
- import/order rule now accepts sort-order option that may be 'ignore' (default) or 'alphabetical'
- Fixes [import/order] sort order within groups #389
This is my first attempt at trying to implement a secondary alphabetical sort on imports. I would appreciate any feedback! |
aa7aca1
to
a2a80b5
Compare
a2a80b5
to
4cbdbb8
Compare
I resolved the test coverage decrease. Not sure why appveyor is failing, but it appears to be happening when trying to run coveralls. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @randallreedjr! Thanks a lot for working on this.
This is a pretty good start, but I have quite a few request changes. Let me know if you disagree, need explanations or guidance :)
docs/rules/order.md
Outdated
@@ -141,6 +141,42 @@ import index from './'; | |||
import sibling from './foo'; | |||
``` | |||
|
|||
### `sort-order: [ignore|alphabetical]`: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bikeshedding: maybe simply call this sort
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
src/rules/order.js
Outdated
@@ -205,6 +269,7 @@ module.exports = { | |||
}, | |||
'Program:exit': function reportAndReset() { | |||
makeOutOfOrderReport(context, imported) | |||
makeOutOfAlphabeticalOrderReport(context, imported, sortOrder) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrap this in a if (sortOrder === 'alphabetical') {}
and remove the latter comparison to ignore
. This will make it clearer when this will be called, and comparing it to 'alphabetical'
will prevent surprises when we add other sort cases later on. Also, you won't have to pass this parameter around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another separate thing:
I think it would be nice to to not re-report the modules that were reported in makeOutOfOrderReport
.
Let's assume lowercase to be part of group 1, uppercase of group 2, and group 1 should appear before group 2.
import 'B';
import 'A';
import 'b';
import 'a';
I haven't tried it yet, but I wouldn't be surprised if there were two error reports for moving B to a latter location or something. I would love some tests on that too, though the problem might not be shown with this example.
The reason I'm worried about this, is when you introduce the order
rule to a new codebase, where imports are sorted in random ways, you'll get a lot of errors for the import section. I'd like for it to be the least confusing people and avoid having 20 errors when you required 10 modules. Note that the code might actually do this already, but without tests I have trouble being sure.
By not re-reporting modules, I think findOutOfAlphabeticalOrder
could be simplified a bit too. I'm a bit confused about its inner workings at the moment.
Also, with only the makeOutOfOrderReport
report, you have a consistent should occur before
/should occur after
message. I think it would be nice if that was true for all of the reports. Maybe compile the whole list with the group and the sort options one way, then do it the other way and take the shorter one.
Sorry to add a lot of work 😅
src/rules/order.js
Outdated
@@ -56,6 +101,21 @@ function makeOutOfOrderReport(context, imported) { | |||
reportOutOfOrder(context, imported, outOfOrder, 'before') | |||
} | |||
|
|||
function makeOutOfAlphabeticalOrderReport(context, imported, sortOrder) { | |||
const outOfOrder = findOutOfAlphabeticalOrder(imported, sortOrder, 'a-z') | |||
if (!outOfOrder.length) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (outOfOrder.length === 0) {}
src/rules/order.js
Outdated
reportOutOfAlphabeticalOrder(context, reversedImported, reversedOrder, 'after', 'z-a') | ||
return | ||
} | ||
reportOutOfAlphabeticalOrder(context, imported, outOfOrder, 'before', 'a-z') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there maybe a way to refactor this function and makeOutOfOrderReport
, they are really similar. Forget it if you deem that it then becomes harder to read.
src/rules/order.js
Outdated
return firstName > secondName | ||
} else { | ||
throw new Error('Invalid alphabetical direction' + direction) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function alphabetical(firstName, secondName, direction) {
if (direction === 'a-z') {
return firstName < secondName
}
return firstName > secondName
}
You won't ever enter the last case, so no need to account for it.
src/rules/order.js
Outdated
@@ -17,6 +17,16 @@ function reverse(array) { | |||
}).reverse() | |||
} | |||
|
|||
function alphabetical(firstName, secondName, direction) { | |||
if (direction === 'a-z') { | |||
return firstName < secondName |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same module could be imported twice, so this should be <=
and >=
. Please add a test for that case :)
src/rules/order.js
Outdated
@@ -17,6 +17,16 @@ function reverse(array) { | |||
}).reverse() | |||
} | |||
|
|||
function alphabetical(firstName, secondName, direction) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Name it something like compareAlphabetically
? Or isBeforeAlphabetically
(berk). They're ugly names, so if you have better ideas, go for it, but at the moment, without looking at the code and by just looking at places where it's called, it's hard to know what exactly this function does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isAlphabetized
?
src/rules/order.js
Outdated
direction | ||
) | ||
const reversedDirection = direction.split('').reverse().join('') | ||
if (alphabetical(importedModule.name, maxSeenAlphabeticalNode.name, reversedDirection)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless the two names are the same, isn't this just equal to resAlphabetical
?
8eeb761
to
ba90260
Compare
8eeb761
to
58da14f
Compare
58da14f
to
15289cc
Compare
@jfmengels I reworked the approach to solving this problem, please have a look and let me know what you think! |
Anything I can do to help with the review of this PR? |
15289cc
to
d2b0921
Compare
This is definitely an addition I've been looking for! It would be great if it could get merged at some point. @randallreedjr just one remark: I suppose the |
d2b0921
to
bca28f3
Compare
@ctavan Good catch, thanks! |
2233e97
to
c0dea5e
Compare
@benmosher @jfmengels This PR has been open for 7 weeks now. Is there anything I can do to help get it merged in? |
@ljharb too late :p i have already rebased the @randallreedjr on my repo branch https://github.com/snoob/eslint-plugin-import/tree/import-sort-order you can pull them :) i changed my mind i ll be more reactive if this PR is synchronized to my repo |
This PR adds an optional configuration that a lot of people want. I don't quite understand the controversy? |
@janpaul123 there's no controversy; it's just that the number of people that want an option is irrelevant if that option can't be provided correctly. |
940939c
to
6a4156d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To reiterate, the changes requested here are still needed for this to go in.
@snoob thanks for the help, i've used your rebase to update this PR. |
@ljharb cool, makes sense. I think it is good to fix problems with this rule before adding more functionality to it, thanks for reiterating that. 👍 I unfortunately really needed this functionality though, and also the autofixing (which this PR doesn't include), so for anyone who comes across this thread and needs that too, I made a small package based on another PR over here: https://github.com/janpaul123/eslint-plugin-import-order-alphabetical It's not perfect but it suited my needs well enough. 🤷♂️ |
@ljharb : What do you think about @janpaul123 work ? Because we have to fix test on this branch and we don't have autofix also |
@snoob any time you have commits you want me to pull into this PR, please ping me here and I'll do so. |
Hi guys, |
Sorry i haven't time to continue my work on this PR :( |
hi @ljharb, I'll be happy to pick up the torch from where it was left. by following the conversation, it seems like the main problem is interleaved code between import groups, but as far as I can tell, that seems resolved. is it something in here specifically that adds a regression? happy to open a new PR for the additional work, or for you to cherry pick from my branch. |
@itajaja posting a link here would be preferable to opening a new PR; indeed, if the interleaving concerns mentioned upthread are covered by tests, this PR, rebased, might be sufficient. It's a bit tricky right now, though, because tests are failing on master. |
@ljharb also interested in this. let me know if any help is needed getting this done |
I think #1105 will do the same job |
Has there been any update on this? I would love if this was thing. |
6a4156d
to
8224e51
Compare
Whoa! It happened! Pumped to be getting this feature- everyone that contributed, thanks! |
Wow, I can't believe it! This was one of my earliest open source pull requests. Thanks @ljharb for merging and everyone else for helping to get this in!!! |
…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>
Just to be sure, this only sorts based on path of import NOT on variable being imported.. So |