Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

ordered-imports rule doesn't sort in the same order as Organize Imports in TypeScript #4063

Closed
aboyton opened this issue Jul 23, 2018 · 9 comments · Fixed by #4064
Closed

Comments

@aboyton
Copy link
Contributor

aboyton commented Jul 23, 2018

Bug Report

  • TSLint version: master
  • TypeScript version: 2.9
  • Running TSLint via: CLI

See microsoft/TypeScript#25114

TypeScript code being linted

import { foo, _bar } from 'baz';

with tslint.json configuration:

{
  "ordered-imports": true,
}

Actual behaviour

It sorts it to import { _bar, foo }, which is different to how TypeScript organises it.

Expected behaviour

No lint error.

The difference seems to be that TypeScript is normalising strings using toUpperCase where as tslint uses toLowerCase.

aboyton pushed a commit to aboyton/tslint that referenced this issue Jul 23, 2018
This makes it consistent with TypeScript's Organize Imports command

Fixes palantir#4063
aboyton pushed a commit to aboyton/tslint that referenced this issue Oct 9, 2018
This makes it consistent with TypeScript's Organize Imports command

Fixes palantir#4063
aboyton pushed a commit to aboyton/tslint that referenced this issue Nov 7, 2018
This makes it consistent with TypeScript's Organize Imports command

Fixes palantir#4063
@Timebutt
Copy link

Timebutt commented Jul 4, 2019

Is there a reason this isn't being picked up? This is a feature I (and I'm guessing many others) have been waiting for for a long time.

@JoshuaKGoldberg
Copy link
Contributor

@Timebutt it's marked as Status: Accepting PRs which means anybody is welcome to send a PR. Including you! 😉

@Timebutt
Copy link

Timebutt commented Jul 4, 2019

I know and I'm looking to contribute, was just wondering if there might be a specific reason why this wouldn't be fixed? Implementing this will be a breaking change, forcing everybody to change code and this might not be desired. If there is nothing preventing us from fixing this, I will try my hand at implementing this for sure!

@JoshuaKGoldberg
Copy link
Contributor

Gotcha, great!

just wondering if there might be a specific reason why this wouldn't be fixed
breaking change

Good call - adding the missing label. We can review a PR and hold off merging it until the next major version, which should be soon.

@aboyton
Copy link
Contributor Author

aboyton commented Jul 5, 2019

We have a PR for it, it's #4064. I'd love to see this merged although personally I just forked months ago.

@Timebutt
Copy link

Timebutt commented Jul 5, 2019

So we actually have the fix, and it was just waiting for a next major version? In that case, it seems like the timing is in our favour, as a new version will be around soon right @JoshuaKGoldberg?

@JoshuaKGoldberg
Copy link
Contributor

Correct! 🙌

@JoshuaKGoldberg
Copy link
Contributor

Removing the Type: Breaking Change label per #4811. Now accepting PRs!

@JeanMeche
Copy link

JeanMeche commented Oct 8, 2019

Thx for the work !

Is the fix expected to ship in the next release ?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants