[New] order: add distinctGroup option

[hyperupcall]

Haiii~!,

This PR aims to close #2292. It implements what is proposed

I'll add that this functionality is really useful for emulating the behavior of TSLint's native ordered-imports option. This is needed for a PR I've made for tslint-to-eslint-config.

In the linked issue:

It seems reasonable to allow a custom pathGroup to be able to opt out of having a newline inserted around it, but I'm not sure what that schema would look like

I did a first pass implementation that seemed good - lemmie know if I need to change anything

Before this gets merged, I would like to add several more tests to ensure everything is working OK - I opened this PR hopefully for some initial feedback

[ljharb]

Looks pretty good!

[LSH0924]

hi there.
This is exactly the function I am looking for. is this draft still alive?

[hyperupcall]

@LSH0924 When I last made an update, I think there were issues with for example alphabetization not working correctly somehow. I don't remember the specifics, but I've been meaning to test it again and see if there is an actual issue or I was just tripping. I'm going to give it another go today or tomorrow, but if there is an issue and I'm not able to fix it this week I'm probably going to close this issue and leave it up to somebody else to continue from here

EDIT: I have identified the issue - my original proposed changes didn't actually implement the feature. I believe I have found the correct implementation - I'll publish my changes very soon after some cleanup

[ljharb]

@hyperupcall thanks! in either case, please do not close the PR; others can update it later instead.

[hyperupcall]

Okay so the feature has been implemented, but on further thought, I think it might be better to fix the original issue in a different way. The original feature request (and mine too) was/is

How can I get a certain import to customly get ordered within its group?

Within the implementation, the imports are ordered within its group. The catch is that patterns that match a particular pathGroup.pattern create a new group. As I understand it, these new groups always have a decimal rank value. But, this doesn't intuitively make sense because the pathGroup.position property doesn't imply a new group is created, only that it is ordered within its own pathGroup.group

The noInsertNewline property partially solves this because it allows for manually disabling newlines and making it appear that everything works. But I have found that it is quite hard to get the matching behavior in various situations (possibly impossible), and it requires extra effort

I propose an additional property called something like pathGroupsNaturalNewlines - it will automatically create newlines the way that makes sense. Moreover, modifications to pathGroups appear to not create an extra group, Below is a demonstration of the current implementation

Here is what a file would currently look like

file.ts

import A from "Bar";
import B from "baz";
import b from "Foo";

import J from ".";

import F from "..";
import D from "../";

import Z from "../Bar";
import O from "../baz";
import Z from "../Foo";

import L from "./Bar";
import P from "./baz";
import L from "./Foo";

This is what the same file would look like if pathGroupsNaturalNewlines behaves as is described and is true:

file.ts

import A from "Bar";
import B from "baz";
import b from "Foo";

import J from ".";
import F from "..";
import D from "../";
import Z from "../Bar";
import O from "../baz";
import Z from "../Foo";

import L from "./Bar";
import P from "./baz";
import L from "./Foo";

Configuration:

.eslintrc.json

"pathGroupsExcludedImportTypes": [],
"pathGroupsNaturalNewlines": true,
"pathGroups": [
    {
        "pattern": "./",
        "patternOptions": {
            "nocomment": true,
            "dot": true
        },
        "group": "sibling",
        "position": "before"
    },
    {
        "pattern": ".",
        "patternOptions": {
            "nocomment": true,
            "dot": true
        },
        "group": "parent",
        "position": "before"
    },
    {
        "pattern": "..",
        "patternOptions": {
            "nocomment": true,
            "dot": true
        },
        "group": "parent",
        "position": "before"
    },
    {
        "pattern": "../",
        "patternOptions": {
            "nocomment": true,
            "dot": true
        },
        "group": "parent",
        "position": "before"
    }
]

If pathGroupsNaturalNewlines solves everyone's use cases, then it might only be necessary to implement that and not go with noInsertNewline after all. @LSH0924 @thany would either of you still have a need for noInsertNewline if pathGroupsNaturalNewlines exists?

Thoughts?

Once we decide what to do, I'll remove the implementations we don't need (if any) and add the necessary documentation and add actual tests

[codecov]

Codecov Report

Merging #2395 (80e838a) into main (a8781f7) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 80e838a differs from pull request most recent head 998655b. Consider uploading reports for the commit 998655b to get more accurate results

@@            Coverage Diff             @@
##             main    #2395      +/-   ##
==========================================
+ Coverage   95.17%   95.19%   +0.01%     
==========================================
  Files          66       66              
  Lines        2799     2808       +9     
  Branches      940      942       +2     
==========================================
+ Hits         2664     2673       +9     
  Misses        135      135              

Impacted Files	Coverage Δ
src/rules/order.js	99.14% <100.00%> (+0.02%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[hyperupcall]

Since this PR is several months old (and now ready), I'll recap and provide all the necessary information:

This PR aims to close #2292, but approaches the solution differently than proposed.

Originally, it was proposed to implement a property for any entry in pathGroup, so that ordering would stay within its own group. That is, when position was specified, this new property would ensure that a "new group" wouldn't get created and would ensure an extra newline would not be generated. The original PR implemented it this way, but it was too fragile and was difficult to use in practice.

Now, with the current code, the property was added at the top level, so it affects all pathGroups entries. This makes it so that every pathGroup with a custom position property will stay within its own group. As a result, the configuration required to fix this issue is more intuitive.

This PR has been undrafted and includes the relevant tests and documentation ^w^

[hyperupcall]

Oops, I see the unresolved connections, and I can't appear to resolve them since I rebased over the latest commits and force pushed the result.

Both seem to be outdated, as I now default to false in the schema definition and the other comment was about an older property name.

[ljharb]

Thanks, that clears it up!

Let's name this option (and the resulting variables) "distinctGroup", and default it to "true" - and include a note in the readme (and a "TODO, semver-major" comment in the code) that the next semver-major will change the default of "distinctGroup" from true to false.