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

Organize Imports doesn't sort '_' as tslint expects it #25114

Closed
henry-alakazhang opened this issue Jun 21, 2018 · 16 comments
Closed

Organize Imports doesn't sort '_' as tslint expects it #25114

henry-alakazhang opened this issue Jun 21, 2018 · 16 comments
Labels
Bug A bug in TypeScript Domain: Organize Imports Issues with the organize imports feature Good First Issue Well scoped, documented and has the green light Help Wanted You can do this
Milestone

Comments

@henry-alakazhang
Copy link

TypeScript Version: 3.0.0-dev.20180620
vscode Version: 1.24.0 (6a6e02cef0f2122ee1469765b704faf5d0e0d859 2018-06-06T17:37:01.579Z)
OS Version: Ubuntu 16.04 LTS

Search Terms: organise organize imports tslint

Code

import { foo, _bar } from 'baz';

Expected behavior: tslint expects the imports to have _ before alphabetic characters, so Organize Imports should sort it to import { _bar, foo }

Actual behavior: It sorts it to import { foo, _bar }, which causes a tslint error.

@mhegazy mhegazy added Bug A bug in TypeScript Domain: Organize Imports Issues with the organize imports feature Help Wanted You can do this labels Jun 21, 2018
@mhegazy
Copy link
Contributor

mhegazy commented Jun 21, 2018

A PR would be appreciated.

@mhegazy mhegazy added this to the Community milestone Jun 21, 2018
@mhegazy mhegazy added the Good First Issue Well scoped, documented and has the green light label Jun 21, 2018
@Shobhit1
Copy link

I can try my hands on this one, if no one else is working on it!

@henry-alakazhang
Copy link
Author

It looks like the issue is just using toUpperCase vs. tslint using toLowerCase, but given this

    /* ...
     * 
     * Case-insensitive comparisons compare both strings one code-point at a time using the integer
     * value of each code-point after applying `toUpperCase` to each string. We always map both
     * strings to their upper-case form as some unicode characters do not properly round-trip to
     * lowercase (such as `ẞ` (German sharp capital s)).
     */

in core.ts:1698, should tslint be changed instead?

@Shobhit1
Copy link

I would agree that changing the core.ts file in a stable sort function will not be a good idea. May be exploring the possibility of a change in tslint is the way to go.

@aboyton
Copy link

aboyton commented Jul 23, 2018

I've raised an issue with TSLint palantir/tslint#4063 and submitted a pull request there palantir/tslint#4064, so let's see what they think about fixing this on their end.

@suhailsinghbains
Copy link

Hi,
I'm new to open-source, can I solve this issue as it is still open?
Thanks

@Shobhit1
Copy link

Shobhit1 commented Oct 5, 2018

I believe that this is being looked into from tslint side of things.

@suhailsinghbains
Copy link

Ohh, okay.
Thanks @Shobhit1

@aboyton
Copy link

aboyton commented Oct 9, 2018

Thanks for the reminder. There was a request on my PR, I've addressed that now and updated the PR. (Personally I just forked the TSLint rule but it would be nicer to get this merged into master.)

@RyanCavanaugh RyanCavanaugh modified the milestones: Community, Backlog Mar 7, 2019
@Lakston
Copy link

Lakston commented Mar 8, 2019

Is this taken care of or should someone (maybe I can ? since it looks like it's tagged 'good first issue') take the lead on this ?

I'm constantly having this problem on an angular project where the MAT_DIALOG_DATA imports are always misplaced.

@aboyton
Copy link

aboyton commented Mar 9, 2019

I think we decided it would be better to address this on the TSLint end but my PR there is waiting a major release despite having a backwards compatibility flag. I’d try bumping there myself. That said TSLint isn’t getting much love recently so I should get back to my PR on ESLint and address the issues that they had so it can be merged typescript-eslint/typescript-eslint#256

@Priyanka-Lenka
Copy link

Is anybody working on it or else i can work on the issue

@aboyton
Copy link

aboyton commented Oct 22, 2019

My PR for TSLint palantir/tslint#4064 to change their order to be consistent with Organise Imports actually got pushed 16 days ago (after more than a year) so once they make a release we should be able to close this issue.

Personally I would love it if TypeScript would break imports onto multiple lines in the same way as Prettier does as running Organise Imports and Prettier both conflict with each other, but maybe that's deserving of a new issue :)

@Maximaximum
Copy link

I think that this issue can be closed now. Tslint (tested on v 6.1.1) now sorts like import { foo, _bar }

@henry-alakazhang
Copy link
Author

Yep, this is no longer a problem with the latest tslint.

@chengyin
Copy link

chengyin commented May 1, 2020

As TSLint getting deprecated, we are trying to move to eslint-plugin-import's import/order, which has the same issue. I filed an issue on their end (import-js/eslint-plugin-import#1742).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Domain: Organize Imports Issues with the organize imports feature Good First Issue Well scoped, documented and has the green light Help Wanted You can do this
Projects
None yet
Development

No branches or pull requests

10 participants