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

Ordered-Imports Rule: Allow sorting by the trailing end of the module path #3178

Merged
merged 6 commits into from Sep 22, 2017

Conversation

berickson1
Copy link
Contributor

PR checklist

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

Overview of change:

Add an additional argument to ordered-import rule allowing sorting by trailing end of path. This allows sorting modules with relative path by filename

CHANGELOG.md entry:

[enhancement] Allow sorting imports by trailing end of path

@palantirtech
Copy link
Member

Thanks for your interest in palantir/tslint, @berickson1! 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.

@@ -104,27 +116,39 @@ const TRANSFORMS = new Map<string, Transform>([
["case-insensitive", (x) => x.toLowerCase()],
["lowercase-first", flipCase],
["lowercase-last", (x) => x],
["full-path", (x) => x],
["module-name", (x) => {
const splitIndex = x.lastIndexOf("/");
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably handle imports from node_modules special. Otherwise sorting would be odd when using submodule imports or scoped modules:

import foo from "a/foo"
import bar from "b/bar"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ajafff is there an easy way to tell when importing from node_modules vs. relative?

Copy link
Contributor

Choose a reason for hiding this comment

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

Take a look at noSubmoduleImportsRule.ts it contains such a condition:

!(ts as any as {isExternalModuleNameRelative(m: string): boolean}).isExternalModuleNameRelative(expression.text) &&

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :)

Copy link
Contributor

@ajafff ajafff left a comment

Choose a reason for hiding this comment

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

Code looks good.
I won't judge if this new option makes sense / is needed, because I don't like this rule at all.

Possible values for \`"module-source-path"\` are:

* \`"full-path'\`: Correct order is \`"./a/Foo"\`, \`"./b/baz"\`, \`"./c/Bar"\`. (This is the default.)
* \`"module-name"\`: Correct order is \`"./c/Bar"\`, \`"./b/baz"\`, \`"./a/Foo"\`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer "full" and "basename"

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

I don't mind adding this option, but please rename the option names to "full" and "basename" as @ajafff suggested.

@adidahiya
Copy link
Contributor

@berickson1 also let me know if you have any issues signing the CLA

@berickson1
Copy link
Contributor Author

Sorry for the CLA delay - I had to get my employer's blessing first

@ajafff ajafff merged commit 126836c into palantir:master Sep 22, 2017
@ajafff
Copy link
Contributor

ajafff commented Sep 22, 2017

Thanks @berickson1

HyphnKnight pushed a commit to HyphnKnight/tslint that referenced this pull request Apr 9, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants