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

Change normalisation of ordered-imports #4064

Merged
merged 2 commits into from
Oct 5, 2019
Merged

Conversation

aboyton
Copy link
Contributor

@aboyton aboyton commented Jul 23, 2018

This makes it consistent with TypeScript's Organize Imports command

Fixes #4063

PR checklist

  • Addresses an existing issue: #0000
  • New feature, bugfix, or enhancement
    • Includes tests
  • Documentation update

Overview of change:

This makes the sorting consistent with what TypeScript's Organise Imports ordering does, see microsoft/TypeScript#25114

Is there anything you'd like reviewers to focus on?

Nothing in particular.

CHANGELOG.md entry:

[bugfix] Make ordered-imports consistent with TypeScript's Organise Imports ordering
[new-rule-option] case-insensitive-legacy for ordered-imports rule

@palantirtech
Copy link
Member

Thanks for your interest in palantir/tslint, @aboyton! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

@johnwiseheart
Copy link
Contributor

Your code itself looks like a pretty simple change, but I have some questions:

Consider the following:

import { foo, _bar } from 'baz';

Before this PR, fixing this code would result in:

import { _bar, foo } from 'baz';

right? Because we were previously enforcing that underscored variables would be at the beginning, I think that makes this a breaking change. Can you confirm @giladgray or @suchanlee?

One fix for this might be to allow either way as an option to the rule, in which case in a future major release we can swap the default from being at the front to at the back.

@aboyton
Copy link
Contributor Author

aboyton commented Jul 24, 2018

Yes, technically this is a breaking change. It is enforcing a different ordering. You could provide an option to allow people to migrate, or you could tell people to run --fix to get the new ordering. Up to you.

@johnwiseheart
Copy link
Contributor

I think that the option is probably the best idea. Are you interested in implementing that? Unfortunately the maintainers don't have a lot of time to develop features other than trying to manage the PRs and issues.

@aboyton
Copy link
Contributor Author

aboyton commented Oct 9, 2018

I've added a option to restore the legacy behaviour as requested. This diff would be much smaller if #4214 is merged first.

Andrew Boyton added 2 commits November 7, 2018 16:05
This makes it consistent with TypeScript's Organize Imports command

Fixes palantir#4063
Not sure this is necessary since we have a `--fix` but if people really want the old ordering they now can.
@elsmr
Copy link

elsmr commented Jan 23, 2019

Hi all, is there anything blocking this PR from being merged? Seems like it has been idle since Nov 2018. This fix would enable us to use this lint rule since a lot of devs use "Organize Imports" from TypeScript 😄

@aboyton
Copy link
Contributor Author

aboyton commented Jan 23, 2019

If it needs rebasing I'm happy to do that.

@JoshuaKGoldberg
Copy link
Contributor

JoshuaKGoldberg commented Jan 25, 2019

Since this is a breaking change, it's blocked on when the next TSLint version (either minor or major - unclear to me) is ready to be released. Unknown yet when that is, sorry!

@elsmr
Copy link

elsmr commented Jan 25, 2019

Ok! thanks for the reply

@JoshuaKGoldberg
Copy link
Contributor

@adidahiya another one with the clean-lockfile CI freeze :o

@adidahiya
Copy link
Contributor

thanks for the PR @aboyton!

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

Successfully merging this pull request may close these issues.

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