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

import/order: don't force new line on specific pathGroup #1746

Open
beatriz opened this issue May 4, 2020 · 12 comments
Open

import/order: don't force new line on specific pathGroup #1746

beatriz opened this issue May 4, 2020 · 12 comments

Comments

@beatriz
Copy link

beatriz commented May 4, 2020

I would like to be able to have a specific pathGroup that doesn't force a newline.

My objective would be to have the imports ordered in the following manner (with react before all, but together with the external imports):

import React from "react";
import PropTypes from "prop-types";
import _ from "lodash";

import {Form, Input, Checkbox} from "antd";

At the moment I have this config:

"import/order": [
  "warn",
  {
    "groups": [
      "builtin",
      "external",
      "internal",
      ["parent", "sibling", "index"]
    ],
    "pathGroups": [
      {
        "pattern": "{antd,@ant-design/icons}",
        "group": "external",
        "position": "after"
      }
    ],
    "pathGroupsExcludedImportTypes": ["builtin"],
    "alphabetize": {"order": "asc"},
    "newlines-between": "always"
  }
]

Which forces me to alphabetize all the imports, to something like this:

import _ from "lodash";
import PropTypes from "prop-types";
import React from "react";

import {Form, Input, Checkbox} from "antd";

The only alternative I find is to add a specific pathGroup for react, with:

{
  "pattern": "react",
  "group": "external",
  "position": "before"
}

But this forces me to have a line between the react import and all other external imports.

Is there a way to have a specific pathGroup without newlines at the end?
As an alternative, I would be ok with not alphabetizing the imports on the external group, but I don't think this is possible either.

@ljharb
Copy link
Member

ljharb commented May 4, 2020

Disabling alphabetization on a specific group (even when it's enabled on all the groups) is absolutely an option that makes sense to me; a PR is welcome.

It also maybe seems reasonable for newlines-between to take, in addition to a string, some kind of option that allows you to invert the setting between specific pairs of groups - but I'm not confident if this is the best approach. Any schema suggestions?

@beatriz
Copy link
Author

beatriz commented May 4, 2020

Disabling alphabetization on a specific group (even when it's enabled on all the groups) is absolutely an option that makes sense to me; a PR is welcome.

Do you prefer to only disable alphabetization on a specific group or to define a specific alphabetization for a group, the same way the global alphabetization is defined? I'm happy to take a go at this!

On the subject of the newline, I thought an alternative could be to define it on the pathGroup setting. The position either be a string ("before"/"after") or an object that also defines a different newline between setting (something like "position": {"pos": "before", "with-newline": false}), which would also override the global newlines-between.
I'm making these suggestions purely from a functional and end-user standpoint, so am not sure if any would have any kind of implementation downside.

@beatriz
Copy link
Author

beatriz commented May 5, 2020

Disabling alphabetization on a specific group (even when it's enabled on all the groups) is absolutely an option that makes sense to me; a PR is welcome.

When I read this the first time, what I understood was disabling alphabetization on a specific pathGroup, but after looking into this a bit more I am not totally sure as you could mean to disable it on whole groups. If your idea was the second, I'm not sure what the appropriate schema would be.

@ljharb
Copy link
Member

ljharb commented May 5, 2020

re the alphabetization: good point; really it needs to apply to the whole group, which does make the schema tricky. We could extend groups so anywhere it can accept a string "some group", it can also accept an object like { "name": "some group" }, and then that object could also support alphabetize (with the same alphabetize schema as at the root of the rule)?

Re the newlines, in a similar way, I think the setting needs to go on the group, not the pathGroup - the point of pathGroup is to alter in which group something is included; it seems much simpler (both in terms of impl, and mental model) to keep configurations to the granularity of groups only. Thoughts?

@beatriz
Copy link
Author

beatriz commented May 5, 2020

I understand your point on the whole logic on groups, and I agree with the part on alphabetization. I think having the groups support objects for additional settings is a good approach.

The newlines part, however, is a bit more tricky, because when we define "newlines-between": "always" and a pathGroup with position, it forces a newline between a group. From my point of view, this is cool and really useful because it allows us to create an additional level of grouping inside of one of the existing groups. However, having the newlines affect an entire group would override this additional separation on a group.
If you take a look on my example above, if the definition were to not have newlines only on the external group, it would force the antd imports to be together with the others, instead of separate as they are at the moment.
I think it wouldn't be bad to define the newlines on a group as the alphabetization, I just think that it would be useful to also have it on the pathGroups.

I'm sorry if I'm only coming up with problems and not much of a good suggestion, but I've only started recently to use this plugin and don't have such a big picture of the use cases 😅

@ljharb
Copy link
Member

ljharb commented May 31, 2020

I'm still not clear on what a good solution would be. I'm happy to review a PR, but I don't want to suggest that just yet because we might discover that all the code needs to be thrown out and replaced with another approach :-)

@sergeushenecz

This comment has been minimized.

@sergeushenecz
Copy link

@beatriz "On the subject of the newline, I thought an alternative could be to define it on the pathGroup setting. The position either be a string ("before"/"after") or an object that also defines a different newline between setting (something like "position": {"pos": "before", "with-newline": false}), which would also override the global newlines-between. I'm making these suggestions purely from a functional and end-user standpoint, so am not sure if any would have any kind of implementation downside."

It is will be good soluion

Disabling alphabetization on a specific group (even when it's enabled on all the groups) is absolutely an option that makes sense to me; a PR is welcome.

Do you prefer to only disable alphabetization on a specific group or to define a specific alphabetization for a group, the same way the global alphabetization is defined? I'm happy to take a go at this!

On the subject of the newline, I thought an alternative could be to define it on the pathGroup setting. The position either be a string ("before"/"after") or an object that also defines a different newline between setting (something like "position": {"pos": "before", "with-newline": false}), which would also override the global newlines-between. I'm making these suggestions purely from a functional and end-user standpoint, so am not sure if any would have any kind of implementation downside.

It is will be good soluion "with-newline": false

@hyperupcall
Copy link
Contributor

Just an FYI if someone is searching for a solution, this was partially fixed in #2395. The distinctGroup determines whether or not newlines should automatically be generated. I initially tried to implement setting for each individual group (within pathGroup), but I found that to be much less intuitive and harder to use, which is why in the PR, it affects all defined groups.

@acezard
Copy link

acezard commented Mar 2, 2023

Interested in this feature.

This is my config:

const config = {
  alphabetize: { order: 'asc' },
  groups: ['builtin', 'external', 'internal', ['parent', 'sibling', 'index']],
  pathGroups: [
    {
      pattern: '{react,react-*,react-*/**}',
      group: 'builtin',
      position: 'before'
    },
    {
      pattern: '{cozy-*,cozy-*/**}',
      group: 'external',
      position: 'after'
    }
  ],
  distinctGroup: true,
  pathGroupsExcludedImportTypes: [
    '{react,react-*,react-*/**,cozy-*,cozy-*/**}'
  ],
  'newlines-between': 'always',
  warnOnUnassignedImports: true
}

It will lint succeed on import order looking exactly like that:

import React from 'react'

import cx from 'classnames'
import PropTypes from 'prop-types'

import Icon, { iconPropType } from 'cozy-ui/transpiled/react/Icon'
import { makeStyles } from 'cozy-ui/transpiled/react/styles'

import Account from '../../assets/icons/Account.svg'

but what I really want is this, as you can see the only difference is that React has no newline after it, but it's still on top, while the cozy-** imports still have their newline after them:

import React from 'react'
import cx from 'classnames'
import PropTypes from 'prop-types'

import Icon, { iconPropType } from 'cozy-ui/transpiled/react/Icon'
import { makeStyles } from 'cozy-ui/transpiled/react/styles'

import Account from '../../assets/icons/Account.svg'

In terms of API I'm not sure, I've seen "with-newline" suggested. I think that would be good for me, but maybe using distinctGroup in pathGroups is nice too?

Like:

    {
     pattern: '{react,react-*,react-*/**}',
     group: 'builtin',
     position: 'before',
     distinctGroup: false
   },

In any case, is this feature something you would like to add? I'm interested in implementing it if we can find a suitable API?

@ljharb
Copy link
Member

ljharb commented Mar 2, 2023

The "help wanted" label indicates that. I agree that a suitable schema would need to be decided in order to implement it.

@BenjaminVilla

This comment was marked as spam.

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

No branches or pull requests

6 participants