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

order rule: add pathGroups option to add support to order by paths #1386

Merged
merged 1 commit into from
Dec 6, 2019

Conversation

Mairu
Copy link
Contributor

@Mairu Mairu commented Jun 20, 2019

Well there are multiple issues and pull requests open regarding the ordering of groups by paths.
Because I saw that the proposed approach in #795 was "accepted" I implemented it this way.

But if @ljharb has changed his mind in the mean time and would prefer something like custom groups as proposed in #1015 (comment) I can also rework this.

@coveralls
Copy link

Coverage Status

Coverage increased (+2.6%) to 97.814% when pulling 5c46fae on Mairu:pathGroups into 58b41c2 on benmosher:master.

@coveralls
Copy link

coveralls commented Jun 20, 2019

Coverage Status

Coverage increased (+0.1%) to 96.405% when pulling 0426f16 on Mairu:pathGroups into 99b3fbf on benmosher:master.

docs/rules/order.md Outdated Show resolved Hide resolved
src/rules/order.js Outdated Show resolved Hide resolved
src/rules/order.js Outdated Show resolved Hide resolved
@Mairu
Copy link
Contributor Author

Mairu commented Jul 3, 2019

Anything else needed for this to be merged? @ljharb

docs/rules/order.md Outdated Show resolved Hide resolved
@fundlg
Copy link

fundlg commented Aug 13, 2019

+1, need this feature

@osdiab
Copy link

osdiab commented Sep 16, 2019

Any reason this has stalled? Would love to use this @ljharb

@zaqqaz
Copy link

zaqqaz commented Oct 14, 2019

Any reason this has stalled? Would love to use this @ljharb

@benmosher, @ljharb ⬆️

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

LGTM overall

CHANGELOG.md Outdated Show resolved Hide resolved
@osdiab
Copy link

osdiab commented Nov 6, 2019

ping on if this is gonna get merged...

@osdiab
Copy link

osdiab commented Nov 18, 2019

@benmosher, @ljharb ⬆️

@oliverdolgener
Copy link

I'd really like to see this getting merged :D

@ljharb ljharb merged commit 0426f16 into import-js:master Dec 6, 2019
@Mairu Mairu deleted the pathGroups branch December 7, 2019 08:05
@osdiab
Copy link

osdiab commented Dec 10, 2019

I was wondering if I was misusing this new rule.

With this this code, I want it to behave as so:

// BEFORE FIX
import { testHelper } from "__tests__/helpers";
import { RecurringStatus } from "@my.company/common/es6/entity/types";
import { ChartistGraph } from "@app/components/common/ChartistGraph";
import { ILineChartOptions } from "chartist";

// AFTER FIX
// this is external, so before all this package's imports
import { ILineChartOptions } from "chartist";

// this is external, but monorepo package, so put it after normal external
import { RecurringStatus } from "@my.company/common/es6/entity/types";

// this is internal, so mixed with anything else internal
import { ChartistGraph } from "@app/components/common/ChartistGraph";

// this is internal but secondary, so after other internal code
import { testHelper } from "__tests__/helpers";

I configured my import/order like so:

        "pathGroups": [
          {
            "pattern": "@app/**",
            "group": "internal"
          },
          {
            "pattern": "__tests__/**",
            "group": "internal",
            "position": "after"
          },
          {
            "pattern": "@my.company/**",
            "group": "external",
            "position": "after"
          }
        ],

But this doesn't mark my BEFORE FIX code as an error at all :P not sure why.

@osdiab
Copy link

osdiab commented Dec 10, 2019

I edited the above for my second try

@ljharb
Copy link
Member

ljharb commented Dec 10, 2019

Please file a new issue so we can track it properly :-)

@jgabriele
Copy link

Thank you ❤️!

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.

9 participants