Skip to content
This repository has been archived by the owner on Aug 18, 2020. It is now read-only.

Enforce new lines between import groups #409

Merged
merged 1 commit into from
Oct 23, 2019

Conversation

keyfer
Copy link
Contributor

@keyfer keyfer commented Sep 16, 2019

This enforces a new line between import groups, from this rule: https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/order.md#newlines-between-ignorealwaysalways-and-inside-groupsnever

The reason I chose this approach is because it enforces what looks to be the side our codebase leans towards:

The effect of running with always (caps at 999)
https://screenshot.click/16-45-j6ozp-g0ol1.jpg

The effect of running with never
https://screenshot.click/16-48-r23ln-jt4u0.jpg

With the addition of this rule the following are true:

Invalid

import fs from 'fs';
import path from 'path';
import index from './';
import sibling from './foo';

Valid

import fs from 'fs';
import path from 'path';

import index from './';

import sibling from './foo';

I attempted to see what this rule would cause if I also ran --fix on web, and this was the result:
https://github.com/Shopify/web/pull/18382

@keyfer keyfer changed the title [WIP] Enforce new lines between import groups Enforce new lines between import groups Sep 16, 2019
@keyfer keyfer changed the title Enforce new lines between import groups [WIP] Enforce new lines between import groups Sep 16, 2019
@keyfer keyfer changed the title [WIP] Enforce new lines between import groups Enforce new lines between import groups Sep 16, 2019
@keyfer keyfer force-pushed the enforce-newline-import-groups branch from e20ada6 to 4b30257 Compare September 16, 2019 20:31
@keyfer keyfer requested a review from BPScott September 16, 2019 20:51
@BPScott
Copy link
Member

BPScott commented Sep 16, 2019

LGTM. As discussed in Slack I think that always is nearer to what usually we try and do.

Work out what's going on with the tests and add a CHANGELOG entry before merging

@cartogram
Copy link
Contributor

@keyfer seems like something is up those tests that is not related to this PR. I am going to take a look.

@cartogram
Copy link
Contributor

@keyfer give this a rebase and I think you will be good to go.

@keyfer keyfer force-pushed the enforce-newline-import-groups branch from 4b30257 to c39d30e Compare October 23, 2019 13:47
@keyfer keyfer merged commit 14dc142 into master Oct 23, 2019
@keyfer keyfer deleted the enforce-newline-import-groups branch October 23, 2019 13:53
@keyfer
Copy link
Contributor Author

keyfer commented Oct 23, 2019

Thanks @cartogram

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

3 participants