Support max-line-length in organize imports

mhegazy commented 6 years ago

Referenced in https://github.com/Microsoft/TypeScript/issues/22974

By the way, I ran "organize imports" on about 800 files. Works really well other than this problem and it puts imports that were previously on multiple lines onto one... so that will break my linting rule for "max-line-length". Luckily it's easy to write a script to investigate and break them back onto multiple lines 😄

mhegazy commented 6 years ago

I would say the correct fix here is to let the formatter split lines greater than certain length.

MartinMa commented 6 years ago

This would be awesome.

So that this:

import { AfterContentInit, Component, ContentChildren, Directive, EventEmitter, HostBinding, Input, ModuleWithProviders, NgModule, OnInit, Optional, Output, QueryList, ViewEncapsulation, forwardRef } from '@angular/core';

becomes this:

import {
  AfterContentInit,
  Component,
  ContentChildren,
  Directive,
  EventEmitter,
  HostBinding,
  Input,
  ModuleWithProviders,
  NgModule,
  OnInit,
  Optional,
  Output,
  QueryList,
  ViewEncapsulation,
  forwardRef
} from '@angular/core';

Thus, respecting max-line-length (tslint).

pelotom commented 6 years ago

The auto-importer also breaks the ordered-imports TSLint rule, which specifies how imports should be broken up into groups and how they should be sorted. It would be nice if it were possible to enable/disable various parts of this organize imports command; in my case I want to disable everything except removing unused imports, because everything else is covered by a combination of TSLint and Prettier.

mhegazy commented 6 years ago

@pelotom that is tracked by https://github.com/Microsoft/TypeScript/issues/23366

pelotom commented 6 years ago

@mhegazy That issue seems to be specifically about the ordering of named imports within a single statement, but doesn't cover

ordering of different import statements relative to one another
groups of imports

or am I missing something?

mhegazy commented 6 years ago

which part you are saying conflict with the tslint rule? i thought the problem is that within the same import declaration, named import bindings are ordered differently by tslint (case insensitive) and by tsserver (case sensitive). are there other issues?

pelotom commented 6 years ago

@mhegazy TSlint also allows configuring the order of import statements relative to one another:

You may set the "import-sources-order" option to control the ordering of source imports (the "foo" in import {A, B, C} from "foo").

Possible values for "import-sources-order" are: -"case-insensitive': Correct order is "Bar", "baz", "Foo". (This is the default.) -"lowercase-first": Correct order is "baz", "Bar", "Foo". -"lowercase-last": Correct order is "Bar", "Foo", "baz". -"any": Allow any order.

And it has an option for configuring groupings of import statements:

You may set the "grouped-imports" option to control the grouping of source imports (the "foo" in import {A, B, C} from "foo").

Possible values for "grouped-imports" are:

false: Do not enforce grouping. (This is the default.)

true: Group source imports by "bar", "../baz", "./foo".

That is, it creates newline-separated groups like this:

import 'a';
import 'b';

import '../a';
import '../b';

import './a';
import './b';

Currently Organize Imports destroys these groupings and mashes them all together.

mhegazy commented 6 years ago

I see, thanks for sharing. i do not think such level of configurability will be in in organize imports the server supports. i think the best solution here is to not use organizeImports, and instead have tslint-vscode-plugin fix your lint errors on save.

pelotom commented 6 years ago

@mhegazy

i think the best solution here is to not use organizeImports, and instead have tslint-vscode-plugin fix your lint errors on save.

Unfortunately that's not viable for removing unused imports, because the vscode plugin for TSLint can't apply rules which require type checking. Besides that, the TSLint rule for no-unused-variable has myriad problems and @egamma has advised people to use TypeScript's native noUnusedLocals and noUnusedParameters instead. See https://github.com/Microsoft/vscode-tslint/issues/185, https://github.com/Microsoft/vscode-tslint/issues/70.

Is there any chance that a "remove unused imports" command could be added to the language server?

mhegazy commented 6 years ago

apply rules which require type checking

that has nothing to do with type checking. the organize imports does not require full type-checking, nor does it require semantic state outside the current file, it uses the find-all-refs logic.

Besides that, the TSLint rule for no-unused-variables has myriad problems and @egamma has advised people to use TypeScript's native noUnusedLocals and noUnusedParameters instead.

i think we are talking about two different things here. 1. what APIs needed from the TypeScript language service to make tslint better, and 2. what level of configuration is needed in organize imports feature

For 1, we would be happy to add additional APIs to enable tslint to be faster/more efficient. for 2, as i noted, the value of adding more configuration drops fairly quickly vs the value added.

I would say start a tslint issue, and we will talk o the tslint core team about what APIs we can enable to simplify this scenario.

Is there any chance that a "remove unused imports" command could be added to the language server?

There is already a suggestion for that, that is marked as unused (see https://github.com/Microsoft/TypeScript/pull/22361), and it comes with a quickfix to remove it, with quickFix all option on it. so an IDE could query for suggestions, and apply all fixes on save if that is needed. not specific to imports though, all declarations.

pelotom commented 6 years ago

that has nothing to do with type checking. the organize imports does not require full type-checking, nor does it require semantic state outside the current file, it uses the find-all-refs logic.

I'm talking about the TSLint no-unused-variable rule. I have no idea why it claims to require type checking, but it does, and type checking rules cannot be run inside VSCode (see the issues I linked). My point is just that there's no way as of right now to remove unused imports in VSCode with TSLint, which is why I was hoping to be able to use the Organize Imports feature to do it :)

There is already a suggestion for that, that is marked as unused (see #22361), and it comes with a quickfix to remove it, with quickFix all option on it. so an IDE could query for suggestions, and apply all fixes on save if that is needed. not specific to imports though, all declarations.

Great, looks like exactly what I want!

Rush commented 5 years ago

Are there any workarounds for this issue?

ZackMFleischman commented 5 years ago

@Rush nothing I know of that doesn't require a small investment of scripting. I use saved vim macros to run the organize imports function and then do a second pass to fix anything that's too long and break it up according to regex rules. This works for one off files while editing.

I imagine you could do something similar in the scripting language of your choice over the entire project.

MrDesjardins commented 5 years ago

Our team has the same issue. We had to turn off the organize import feature. We are using Prettier which suit us well for formatting the code, however, it gets in conflict with the organize import when it is used to be fired when a file is saved.

The idea of having the organize import on save it to automatically have the unused import taken out without having human interaction. However, having the organize import and Prettier make the code to be on a single line once organized, and on multiple lines when Prettier is cleaning its part of the code. I haven't investigated why, but prettier seems to be executed first, which cause the code to be formatted correctly but then VsCode organizes and put back into a single line the import, hence causing issues of consistency.

tfburton commented 5 years ago

I'm sad that I just had to turn off organize imports. My team doesn't want a lint rule for the imports, but I like them to be organized. They are ok with me organizing them, but they don't want to be bothered with it. I was depending on this feature until I couldn't get multi-line imports to be happy.

crashmstr commented 5 years ago

Seems like this quickly got off the topic of line length and moved on to ordering of imports :( There was a similar item that was marked as a duplicate of this one, but is there still attention here for just the concern over being able to limit line length when using organize imports? Line length and ordering of the imports seems like they should be two separate topics.

ZackMFleischman commented 5 years ago

I'm for the clear separation.

For me, I can't use the feature at all because the it breaks my linter line length rule (I use 120 typically). Providing a setting where we could adjust that number would allow me to use it without creating a linter error most of the time.

The custom ordering for me would be icing on the cake, but waaaay less priority as the line length as far as feature usability. (at least for my typical use cases).

On Thu, Mar 21, 2019, 8:45 AM crashmstr notifications@github.com wrote:

Seems like this quickly got off the topic of line length and moved on to ordering of imports :( There was a similar item that was marked as a duplicate of this one, but is there still attention here for just the concern over being able to limit line length when using organize imports? Line length and ordering of the imports seems like they should be two separate topics.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/Microsoft/TypeScript/issues/22991#issuecomment-475214404, or mute the thread https://github.com/notifications/unsubscribe-auth/AEGYaXuUSq-_yRrCe9Q-24Xf3swHykrjks5vY372gaJpZM4TAwli .

zpdDG4gta8XKpMCd commented 5 years ago

tslint has an option to ignore max line in imports you guys must be joking all around

zpdDG4gta8XKpMCd commented 5 years ago

you scare me people:

https://palantir.github.io/tslint/rules/max-line-length/

PS

and the reason why you scare me because an issue that is in a domain of tslint in the first place (and already solved there btw), gets pulled out for the next sprint

alexey-kozlenkov commented 5 years ago

@aleksey-bykov do you confuse tslint rule with typescript language service feature?

dsherret commented 5 years ago

@aleksey-bykov it's certainly in the domain of the language service's formatter, but not necessarily a feature that will be implemented. No need to be so spooked out.

(IMO, time would probably be better spent leaving this to other code formatters)

zpdDG4gta8XKpMCd commented 5 years ago

let me say this:

we are still talking about the max-length problem (as the title suggest), don't we?
if so, then this problem simply does not exist, since tslint can ignore it
if no, i personally don't want any formatting to the imports, because:
- i have tons of them, and each line counts, so i wish they were as flat as they can be, because...
- ... i hate scrolling through them
- ... and i stopped caring about imports about 2 years ago since
  1. auto imports
  2. auto imports cleanup
  3. auto imports sorting
so i simply don't look at them and i am glad i don't have to, and it will be a bad day if they start getting bloated because nicely formatted

RyanCavanaugh commented 5 years ago

@aleksey-bykov When we look at issues with no thumbs up, you go into those issues and complain about it, but when we look at issues with 27 thumbs ups, you go into those issues and poo-poo their value too. This doesn't feel like good-faith engagement and I think it'd be best if you were more intentional about where you choose to weigh in on things.

crashmstr commented 5 years ago

@aleksey-bykov I don't want to remove or make exceptions for the tslint rule if at all possible, but I appreciate that I could and that for some, that may be their preferred solution.

I would like if VS Code could accommodate a tsline line length rule (or rule specified in VS Code) that does not have exceptions for imports, because for now I manually run the command to organize imports then edit for length (as that would not work for organize on save unless I do make exceptions for the tslint rule).

zpdDG4gta8XKpMCd commented 5 years ago

@RyanCavanaugh i apologize for being a stink bug, this is what i am, this is what i do, didn't mean to pick on you

dsherret commented 5 years ago

@aleksey-bykov you've made your preference clear. Some other peoples' preference is for all code to fit within their screen's width to prevent needing to horizontally scroll regardless of whether they are using tslint—having the import statements automatically format would make the editing experience a little nicer for those people because they wouldn't need to fix this every time they run organize imports.

If anything, I think it would be more productive to ask for this to be configurable (which it likely will be if implemented) so you can turn it off.

zpdDG4gta8XKpMCd commented 5 years ago

@dsherret i see, ok, although it's not what the original problem is about (and because of the fact it is not), i buy it

dsherret commented 5 years ago

@aleksey-bykov I think it's related to the original problem (I'm the one quoted in it). This isn't really tslint related... think about what the motivation for that tslint rule is and transitively that's what the original post is about.

Additionally, the API and tsserver don't care about tslint. This feature if implemented would likely be a setting specified on FormatCodeSettings or its base (see here). Then vscode and other editors can provide that value however they please.

Anyway, this is a bit of a digression... (my apologies)

vinagreti commented 5 years ago

VSCode Workaround for those who are having problem with format on save + sort import You can save the file, wait for it to be sorted, then you can break the long import line and than use the "Save without Formatting" option of vscode. Hit ctrl+p and type Save without Formatting.

bduffany commented 4 years ago

If you're like me, you like Prettier and VS Code but don't like how they clash when handling imports.

I've found a solution for VS Code that's been working very nicely, keeping the super fast formatting speed of Prettier while keeping imports tidy using eslint-plugin-import.

Check out the .eslintrc, .eslintignore, .vscode/settings.json, .prettierrc, and any devDependencies containing "typescript" or "eslint" in .package.json of https://github.com/bduffany/nextjs-app-template

laurensnl commented 4 years ago

I spent so much time getting it right (and failed!)... Thank for sharing your config @bduffany! Your solution seems to work perfectly!

Elias-Graf commented 4 years ago

I haven't read through everything and don't get why there isn't an option for this yet. I'm not using prettier (So I think I'm using the default formatter? It's all a bit confusing when you're not completely into this kind of stuff) but es-lint with the typescript plugin. Couldn't we just say that we make a setting (for example: "source.organizeImportsMaxLen": 100) and break imports into one per line? Just provide the option and people can turn it on or off if they please.

Additionally, I'd suggest if you want something different like filing the line and only breaking when the import doesn't fit on it, you should have to use a custom formatter.

What I mean:

import {bla, bla, bla, bla
    bla, bla, bla, bla, bla,
    bla} from './bla';

This is also affecting exports. My multi-line export statement is being turned into one line:

export {
    Core,
    TokenKeyword,
    TokenSymbol,
    Token,
};

is being turned into a single line. Funny thing, export default {...}; does not have the same issue.

I'm also using "source.fixAll": true" if that makes a difference. Hopefully, I got everything correct and didn't make a complete fool of myself :D

Edit: Why not just add a few more options so everyone can configure this as they pleases:

sortImports
deleteUnusedImports
importLineMode (I'd suggest options like: auto, mutli-line, single-line, manual / off)

daidodo commented 4 years ago

Hi, for those who is still struggling on those issues, please try JS/TS Import Sorter which I believe covers all following topics discussed here:

Max-line-length
TSLint/prettier compatible
Groups of imports
Removal of unused imports

If you do find bugs or features not supported, I'm happy to accept open issues as a maintainer.

Thanks!

Elias-Graf commented 4 years ago

@daidodo Works like a treat well done! It's also really configurable.

Question is, maybe this should work out of the box but it's not up to me to decide this.

tommueller commented 3 years ago

Any updates on this?

thinkjson commented 3 years ago

We ended up just using both organize imports and prettier on save.

Bessonov commented 2 years ago

I vote for support "one import/export statement per line with trailing comma", like:

import {
  AfterContentInit, <-- one statement per line
  Component,
  ContentChildren,
  Directive, <-- trailing comma, so additions after this line doesn't affect this line
} from '@angular/core';

instead of max-line-length property. This plays nicely with diffing, merging and code review.

Rush commented 2 years ago

Clearly one size doesn't fit all so why even vote? :)

microsoft / TypeScript

Support max-line-length in organize imports #22991