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

Add absolute-first-group option to imports-first #246

Closed
wants to merge 2 commits into from
Closed

Add absolute-first-group option to imports-first #246

wants to merge 2 commits into from

Conversation

singles
Copy link
Contributor

@singles singles commented Apr 18, 2016

As we're trying to enforce formatting rules for imports like below:

import a from `absolute-import`
//new line here
import b from `./relative-import`
import c from `./../../another-relative-import`
//new line here
const whateverElse = '...';

this is addition to previously created #245. I have doubts about option name though - it is in fact some kind of visual grouping, yet it doesn't speak to me in 100%.

@benmosher
Copy link
Member

I think this may be covered by the import-order rule pending merge from https://github.com/jfmengels/eslint-plugin-import-order. cc @jfmengels

@jfmengels
Copy link
Collaborator

jfmengels commented Apr 18, 2016

Yeah, this behaviour is covered by import-order, coming soon in this repo (will work on it tonight), or available https://github.com/jfmengels/eslint-plugin-import-order as @benmosher said.

@singles
Copy link
Contributor Author

singles commented Apr 18, 2016

@jfmengels I tried to configure eslint-plugin-import-order to get behaviour described in this PR, but couldn't make it to work like in mentioned example :/ Ok, closing this PR then.

@singles singles closed this Apr 18, 2016
@jfmengels
Copy link
Collaborator

Ah sorry. I thought you only wanted to group the same "type" of imports together, but you want that and also to get the newline between each group? If so, my bad, and no, that is not supported. The rule only the grouping and ordering of the groups, no ordering inside of the groups nor adding lines between the groups.

@singles
Copy link
Contributor Author

singles commented Apr 18, 2016

@jfmengels Yeah, I could make a PR to eslint-plugin-import-order but if you guys are saying that it should land here in near future I can hold.

Basically I was think about two kind of options for that:

  1. force-new-line which allows you to force adding new line after each group (which would duplicate Add newline-after-import rule #245). But in order to be able to achieve absolute/relative grouping, there comes 2nd options
  2. ability to configure groups, so instead of:
"import-order/import-order": [2, {"order": ["index", "sibling", "parent", "external", "builtin"]}]

you would have eg. :

"import-order/import-order": [2, {"order": [
    [ "builtin", "external" ],  //first group
    [ "index", "sibling", "parent" ] //second group
}]

Having it configured that way gives you huge amount of flexibility and IMHO is still very readable.

@jfmengels
Copy link
Collaborator

but if you guys are saying that it should land here in near future I can hold

Yeah, you should hold ;)

force-new-line-between-groups

Sure, I'm fine with it (as long as it's not a default). The name can a bit shorter, like line-between-groups.

ability to configure groups

I thought of the same thing, and I like it. The only downside I see, is that it loosens the rule schema, and unexpected arguments can then slip in. Other than that, I think this is nice.

(cc @sindresorhus who might be interested in the discussion)

@singles
Copy link
Contributor Author

singles commented Apr 18, 2016

The only downside I see, is that it loosens the rule schema, and unexpected arguments can then slip in.

What kind of arguments do you have in mind?

@jfmengels
Copy link
Collaborator

Right now, The order param needs to be an array of unique 5 strings, that must be "index", "sibling", "parent", "external" or "builtin". That means (afaik) that there is no way to forget a type, make a typo, have duplicates, etc. If we're switching to the group ordering, there is no way (with JSON schema, but we can do it manually) to enforce the same restrictions. Types can be omitted, be duplicated, have a typo...

@singles
Copy link
Contributor Author

singles commented Apr 18, 2016

@jfmengels Ah, got it. Currently I cannot think of a better way of defining those groups, but maybe I will figure out something in the meantime.

Hey @lo1tuma, we could use your expertise here - could you help ?:)

@jfmengels jfmengels mentioned this pull request Apr 18, 2016
4 tasks
@lo1tuma
Copy link
Contributor

lo1tuma commented Apr 19, 2016

What about making a separate rule for import grouping, e.g. newline-after-import-group.

Which could be configured as newline-after-import-group: [ "error", { groups: [ [ "builtin", "external" ], [ "index", "sibling", "parent" ] ] } ]

So the following would be considered as a problem:

import url from 'url';
import express from 'express'; // <-- missing new line after this statement
import foo from './foo';

This would not be considered as a problem:

import url from 'url';
import express from 'express';

import foo from './foo';

import http from 'http';

As you can see this rule doesn’t care about the import order, so there are two import groups that import builtin modules, but they are still separated by a new line.

This would also supercede #245 when configured with only on group including all types of imports [ "error", { groups: [ [ "builtin", "external", "index", "sibling", "parent" ] ] } ].

@jfmengels
Copy link
Collaborator

I feel that a new rule would need to have almost all of the same logic that is in import-order (PR opened btw #247).
Also, the rule would only make sense if groups are correctly respected (at least partially), otherwise you may get errors like this.

import url from 'url'; // <-- missing new line after this statement
import bar from './bar'; // <-- missing new line after this statement
import express from 'express'; // <-- missing new line after this statement
import foo from './foo'; // <-- missing new line after this statement
foo();

Also, I don't see why someone would want to have the newline rule on and not import-order.
I feel that this is a one-step further kind of a thing, rather than an unrelated rule, that's why I'd be more in favor of adding an option than a new rule for it.

@lo1tuma
Copy link
Contributor

lo1tuma commented Apr 19, 2016

I feel that a new rule would need to have almost all of the same logic that is in import-order

Well the logic could be extracted to a separate file which could be used by both rules.

Also, the rule would only make sense if groups are correctly respected (at least partially), otherwise you may get errors like this.

Personally I also don’t have this need but I could imagine that some people don’t care about the order but just want to visually separate the different kind of groups.

@jfmengels
Copy link
Collaborator

Well the logic could be extracted to a separate file which could be used by both rules.

Absolutely.

Personally I also don’t have this need

Me neither. I would actually prefer to have a rule that disallows empty lines between imports.

some people don’t care about the order but just want to visually separate the different kind of groups

If we take the following invalid example:

import url from 'url'; // <-- missing new line after this statement
import bar from './bar'; // <-- missing new line after this statement
import express from 'express'; // <-- missing new line after this statement
import bar from '/bar';
import foo from './foo'; // <-- missing new line after this statement
foo();

the valid, fixed (without re-ordering) version would be this:

import url from 'url';

import bar from './bar';

import express from 'express';

import bar from '/bar';
import foo from './foo';

foo();

which doesn't make much sense IMO. There are separations between "groups", but they just feel kind of random.

@singles singles deleted the imports-first-group-option branch May 10, 2016 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants