import-js / eslint-plugin-import

ESLint plugin with rules that help validate proper imports.
MIT License
5.46k stars 1.56k forks source link

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

Open beatriz opened 4 years ago

beatriz commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 2 years ago

What status this ticket?

sergeushenecz commented 2 years ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

BenjaminVilla commented 4 months ago

Hola donde te encuentras ahora