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

Adding new group types and matching pattern for sort order #1076

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

6graNik
Copy link

@6graNik 6graNik commented Apr 11, 2018

I added support for couple of new types.

One that was internally used absolute and one new private

They both for more flexible order customazing, we need it in our project and I thought maybe some one else will need it to.

Both properties have on pattern field that except regex.

@coveralls
Copy link

coveralls commented Apr 11, 2018

Coverage Status

Coverage increased (+0.02%) to 96.499% when pulling 33cda36 on 6graNik:new-group-types into cfd4377 on benmosher:master.

@ljharb
Copy link
Member

ljharb commented Apr 11, 2018

Absolute has a meaning, this seems fine. I have no idea what “private” means, nor why it needs to be a default group when you can create custom ones.

Either way, please update the documentation to describe all your changes.

@6graNik
Copy link
Author

6graNik commented Apr 11, 2018

@ljharb How can we create custom types? May be I missed it in docs. But right now I only see The only allowed strings are: "builtin", "external", "internal", "parent", "sibling", "index" (https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/order.md)

I created them because I thought we can't create custom ones. And my idea was to separate private company/project package imports from external

I think you are right that we don't need them as a default.

I will update docs accordingly.

@ljharb
Copy link
Member

ljharb commented Apr 11, 2018

Ah, you’re right, I’m thinking of a different rule in a different plugin.

What would you expect “private” to mean, given that this isn’t a standard concept in node?

@6graNik
Copy link
Author

6graNik commented Apr 11, 2018

@ljharb As I said before in my idea private type imports is for company or project level packages/libs, to separate them logically from all external types

@6graNik 6graNik force-pushed the new-group-types branch 2 times, most recently from 1fcb2d0 to 8691ddf Compare April 11, 2018 17:44
@6graNik
Copy link
Author

6graNik commented Apr 17, 2018

I think something wrong with your CI because locally all test completes successfully.

@chenders
Copy link

@6graNik FWIW, when I run the tests on your branch I get the same errors that are happening in the CI.

@lucasveigaf
Copy link

This PR would solve my use case, which is to group company projects. There are some corner cases, which can only be solved by being able to register a group via regex. Any progress on this?

@jeffshaver
Copy link
Contributor

@ljharb thanks for the response and pointing me to this issue. Is there something that needs to be done to get this in? Anything I can do to help?

@ljharb
Copy link
Member

ljharb commented Feb 20, 2019

See #1076 (comment)

In other words, I'm fine to add builtin groups that have a reasonable universal meaning; "private" has none.

@jeffshaver
Copy link
Contributor

@ljharb after reading the docs again and reading the source for the order rule, can you correct me if I'm wrong? Does the "internal" group refer to things that basically look like node modules (no relative paths or whatever) but don't exist in the node_modules folder? If so, I think that group solves the Typescript alias problem and we wouldn't require any changes to switch to this.

@6graNik
Copy link
Author

6graNik commented Feb 20, 2019

If your concern only, about naming, we can defenatly change it from private to whatever. I am not a native English speaker, so may be someone could suggest any proper naming.

Basically we want to add opportunity to group "some" imports, to separate them from other predefined ones. I need it to group some out company projects.

@jeffshaver

This comment has been minimized.

@ljharb
Copy link
Member

ljharb commented Feb 20, 2019

@6graNik you can create a custom group already; why is that not sufficient?

@jeffshaver
Copy link
Contributor

@ljharb is that documented somewhere?

@ljharb
Copy link
Member

ljharb commented Feb 23, 2019

ah, perhaps not; in which case, would a feature to create custom groups be a better solution for this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

7 participants