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 sort-imports member sorting into a separate option #11019

Closed
remcohaszing opened this Issue Oct 25, 2018 · 11 comments

Comments

Projects
None yet
5 participants
@remcohaszing
Copy link
Contributor

remcohaszing commented Oct 25, 2018

What rule do you want to change?

sort-imports

Does this change cause the rule to produce more or fewer warnings?

Fewer

How will the change be implemented? (New option, new default behavior, etc.)?

Currently, sort-imports does two checks. One is always on, the other is optional. I would like to only use the optional part.

One part is that it checks the order of import statements. This can’t be turned off. I would like to check this using import/order, which offers more advanced features.

The part of the rule I would like to use, is the sorting of the imported members.

My proposal is to split the distinct memberSort option into a separate rule: sort-import-members.

Please provide some example code that this change will affect:

// Builtins
import { join, resolve } from 'path';

// External
import { map } from 'lodash';

What does the rule currently do for this code?

sort-imports will cause conflicts with import/order, but also make sure import members are sorted nicely.

What will the rule do after it's changed?

sort-imports should be disabled to prevent conflicts with import/order.

sort-import-members will ensure the imported members are sorted, as sort-imports does now.

@eslint eslint bot added the triage label Oct 25, 2018

@kaicataldo

This comment has been minimized.

Copy link
Member

kaicataldo commented Oct 27, 2018

Given the pluggable nature of ESLint and the size of the plugin community, we can't always ensure that all rules work with plugins. I'm not convinced we need an entirely new rule to solve this problem.

I wonder if a better solve would be to deprecate the ignoreMemberSort option in favor of an option that allows the toggling of sorting import declarations and imported members individually. I'm imagining something like:

{
    "sort-imports": ["error", {
        "sortDeclarations": true,
        "sortMembers": true,
    }]
}

If we decide to go down this route, we should do this in a backwards compatible way.

@remcohaszing

This comment has been minimized.

Copy link
Contributor

remcohaszing commented Oct 28, 2018

That proposal seems just as good to me. 👍

For backwards compatibility, I suggest the following signature:

{
  "sort-imports": ["error", {
    ignoreMemberSort: false,  // Already exists
    ignoreDeclarationSort: false,  //  Analogous to `ignoreMemberSort`. The default matches the current behavious.
    ...otherOptions
  }]
}

@nzakas nzakas changed the title Extract sort-imports member sorting into a separate rule Add sort-imports member sorting into a separate option Oct 29, 2018

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Oct 29, 2018

The proposal to add an option to turn off declaration sort seems reasonable to me. 👍

@remcohaszing are you will to submit a pull request for this proposal?

@remcohaszing

This comment has been minimized.

Copy link
Contributor

remcohaszing commented Oct 29, 2018

I'll give it a try somewhere this week.

remcohaszing added a commit to remcohaszing/eslint that referenced this issue Oct 30, 2018

Update: Add sort-imports ignoreDeclarationSort (fixes eslint#11019)
This allows users to enforce sorting import members, without having to enforce
sorting of import declarations.
@Fandekasp

This comment has been minimized.

Copy link

Fandekasp commented Dec 6, 2018

Can someone look into the related PR ? There hasn't been any action taken in over a month, and I'm also very interested in having a rule to check ordered and sorted imports.
Thank you for your PR @remcohaszing :)

@nzakas nzakas added the tsc agenda label Dec 6, 2018

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 6, 2018

Thanks for the ping, we'll take a look.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 6, 2018

TSC Summary: This proposal seeks to add another option, ignoreDeclarationSort to sort-imports to allow turning off a part of the rule that previously was required. The option is backwards-compatible and there is already a pull request (#11040) for implementation. This issue fell off of the radar and so is missing feedback, but I will champion.

TSC Questions:

  • Should this proposal be accepted?
  • Should #11040 be merged?
@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Dec 9, 2018

Rule changes do not require TSC consent, unless they are breaking. In that case, it looks like there's a champion and 1 up vote already on the proposal. It's now up to the champion to gather more support from the rest of the team, until this issue can be accepted.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 10, 2018

@ilyavolodin oh right, duh! Thanks.

@nzakas nzakas removed the tsc agenda label Dec 10, 2018

@remcohaszing

This comment has been minimized.

Copy link
Contributor

remcohaszing commented Dec 20, 2018

Is any action required from me? I thought not, but it’s taking much longer to get merged than I expected.

@nzakas nzakas added accepted and removed enhancement labels Dec 20, 2018

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 20, 2018

@remcohaszing thanks for the ping. Sorry, a lot of team members are taking time off around this time of year, so things can take a bit longer than usual. It looks like enough team members support this change so I'm marking as accepted. The next step is to review the PR.

btmills added a commit that referenced this issue Jan 4, 2019

Update: Add sort-imports ignoreDeclarationSort (fixes #11019) (#11040)
* Update: Add sort-imports ignoreDeclarationSort (fixes #11019)

This allows users to enforce sorting import members, without having to enforce
sorting of import declarations.

* Docs: fix typo in documentation for sort-imports
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment