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

[sort-type-union-intersection-members] Line feeds are removed when autofixing #4591

Closed
3 tasks done
islandryu opened this issue Feb 25, 2022 · 2 comments
Closed
3 tasks done
Labels
package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin working as intended Issues that are closed as they are working as intended

Comments

@islandryu
Copy link
Contributor

islandryu commented Feb 25, 2022

  • I have tried restarting my IDE and the issue persists.
  • I have updated to the latest version of the packages.
  • I have read the FAQ and my problem is not listed.

Repro

{
  "rules": {
    "@typescript-eslint/sort-type-union-intersection-members": "warn"
  }
}
type T4 =
  | [1, 2, 4]
  | [1, 2, 3]
  | { b: string }
  | { a: string }
  | (() => void)
  | (() => string)
  | 'b'
  | 'a'
  | 'b'
  | 'a'
  | readonly string[]
  | readonly number[]
  | string[]
  | number[]
  | B
  | A
  | string
  | any;

Expected Result

Should be fixed to:

type T4 =
  | A
  | B
  | number[]
  | string[]
  | any
  | string
  | readonly number[]
  | readonly string[]
  | "a"
  | "a"
  | "b"
  | "b"
  | (() => string)
  | (() => void)
  | { a: string }
  | { b: string }
  | [1, 2, 3]
  | [1, 2, 4];

Actual Result

type T4 =
  A | B | number[] | string[] | any | string | readonly number[] | readonly string[] | 'a' | 'a' | 'b' | 'b' | (() => string) | (() => void) | { a: string } | { b: string } | [1, 2, 3] | [1, 2, 4];

Additional Info

releated to #4569
Versions

package version
@typescript-eslint/eslint-plugin 5.12.1
@typescript-eslint/parser 5.12.1
TypeScript 4.5.5
ESLint 8.9.0
node 12.16.0
@islandryu islandryu added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look labels Feb 25, 2022
@Josh-Cena
Copy link
Member

I don't think this is problematic, unless there's a trivial fix. From the ESLint spec:

Each suggestion should focus on a singular change in the code and should not try to conform to user defined styles. For example, if a suggestion is adding a new statement into the codebase, it should not try to match correct indentation, or conform to user preferences on presence/absence of semicolons.

As long as it doesn't cause semantic differences, the fix is accepted.

@bradzacher
Copy link
Member

Yeah this is working as intended. As Joshua pointed out - fixers need not care for formatting, as long as the resulting code is valid.

Adding things like newlines sounds trivial - however it is actually a lot more difficult than it might sound!

First of all - what newlines should be inserted? Sure you can think of cases like:

type T = 
  | A
  | B
  // ...

Which are trivial - each type on a newline. But what about these cases:

type T = A
  | B
  // ...
type T = A | B | C
  | D | E | F;

Now what should the rule do here? There's no "one-size-fits-all" algorithm for this - you have to pick one way of doing things and that's what everyone gets for newlines - which ofc will lead someone to being unhappy.

Next - indentation is important! If you add newlines they have to be correctly indented. It's really trivial for a human to decide how to indent something - but it's really hard to do it algorithmically.
For example:

type T =
/* comment */| A
  | B;

What's the expected indentation here? If we used the indentation of the first type then we'd use 0 characters and it'd all be wrong!
There's no hard-and-fast rule here because a union/intersection can be used in many positions and many ways.
So again - you have to make assumptions and guess at what it might be - which could easily lead to you adding the wrong indentation.

Finally there's things like the positioning of the |/& character. Does it go on the start or end of the newline? Who says which way is right? Again unless you want it to be configurable - you're going to be wrong somewhere.


Long winded way of saying that this is working as intended.

@bradzacher bradzacher added working as intended Issues that are closed as they are working as intended and removed triage Waiting for maintainers to take a look labels Feb 25, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin working as intended Issues that are closed as they are working as intended
Projects
None yet
Development

No branches or pull requests

3 participants