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.

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

import/order Fix group ranks order when alphabetizing #2674

merged 1 commit into from Jan 14, 2023


Copy link

@Pearce-Ropion Pearce-Ropion commented Jan 13, 2023

Resolves #2671

Correctly sorts groupRanks in numerical order when alphabetizing. This bug would occur for groups where the rank overflowed to double digits (eg. "10" is sorted before "4"). This is a bug with the original code and not necessarily caused by #2506. There just previously wasn't a case where enough groups were added to get to the double digit ranks.

The test case I made for this is similar to the example case shown in #2671 but it isn't the most general test case. It's just a case that would fail without this change.

I also went ahead and added a description comment to my previous PR's test case since it was missing one

Copy link

ljharb commented Jan 13, 2023

A test case that fails without this fix is definitely necessary, otherwise we'll just break it again in the future :-)

Copy link

ljharb commented Jan 13, 2023

Unfortunately this test case passes on main :-/ do you have one that fails on main?

Copy link
Contributor Author

Pearce-Ropion commented Jan 13, 2023

Unfortunately this test case passes on main :-/ do you have one that fails on main?

Huh... Oh, it probably has to do with the number of path groups again...

Copy link
Contributor Author

Unfortunately this test case passes on main :-/ do you have one that fails on main?

Ok, this should be working now. I tested it on main too to make sure it failed. I forgot that the only way to get the ranks into the double digits is if you have a lot of groups. Thats what I get for trying to cleanup a test case to make it presentable...

Copy link

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

src/rules/order.js Outdated Show resolved Hide resolved
@ljharb ljharb force-pushed the pearce/order-alphabetize-sort branch from 1a44ca9 to 0778b03 Compare January 13, 2023 21:52
@ljharb ljharb merged commit 0778b03 into import-js:main Jan 14, 2023
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Jan 19, 2023
This PR contains the following updates:

| Package | Type | Update | Change |
| [eslint-plugin-import]( | devDependencies | patch | [`2.27.4` -> `2.27.5`]( |


### Release Notes


### [`v2.27.5`](;2275---2023-01-16)

[Compare Source](import-js/eslint-plugin-import@v2.27.4...v2.27.5)

##### Fixed

-   \[`order]`: Fix group ranks order when alphabetizing (\[[#&#8203;2674](import-js/eslint-plugin-import#2674)], thanks \[[@&#8203;Pearce-Ropion](])



### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.


 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box


This PR has been generated by [Renovate Bot](

Co-authored-by: cabr2-bot <>
Reviewed-by: Epsilon_02 <>
Co-authored-by: Calciumdibromid Bot <>
Co-committed-by: Calciumdibromid Bot <>
@Pearce-Ropion Pearce-Ropion deleted the pearce/order-alphabetize-sort branch January 22, 2023 02:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet

Successfully merging this pull request may close these issues.

[import/order] Potential regression in sibling and parent behavior
2 participants