Skip to content

Conversation

andrewbranch
Copy link
Member

Fixes #43141

@typescript-bot
Copy link
Collaborator

Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping @sheetalkamat, @amcasey, @mjbvz, @minestarks for you. Feel free to loop in other consumers/maintainers if necessary

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Sep 23, 2022
Comment on lines +585 to +587
/** @deprecated Use `mode` instead */
skipDestructiveCodeActions?: boolean;
mode?: OrganizeImportsMode;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think skipDestructiveCodeActions was vaguely named due to aspirations of being used in more than one place, but it didn’t end up that way. FWIW, VS never picked this argument up, so it’s likely only VS Code that uses it.


export const enum OrganizeImportsMode {
All = "All",
SortAndCombine = "SortAndCombine",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the PR focuses on the RemoveUnused... I'm more excited about SortAndCombine :P I see that skipDestructiveCodeActions was a thing (didn't know about it), gonna give it a try in my projects for now

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, to be clear, SortAndCombine ought to be identical to setting skipDestructiveCodeActions which is already set in a VS Code code action called sortImports. (It’s not very discoverable because I learned recently that these code actions lack autocomplete support in the VS Code settings.json files.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ye, I got the fact that this one isn't a new option - but I had no idea that it was already a thing. So I'm glad that I've discovered that old option through this PR :P

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't figure out how to actually use this old option in VS Code. I've found this in the source code:
https://github.com/microsoft/vscode/blob/3a8b1fe03ebbcf57fb9c50b161db91229e2fe04a/extensions/typescript-language-features/src/languageFeatures/organizeImports.ts#L50

I've tried a lot of combinations with _typescript.*, typescript.*, source.* and *.organizeImports.sortOnly: true but none of those work for me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  "editor.codeActionsOnSave": {
    "source.sortImports": true
  }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe Organize Imports is a VS Code standard command, and so automatically appears in the command palette when registered. These variations are registered so can be triggered by ID in configurations like codeActionsOnSave, but I think we would need to add them to the contributes of the extension package.json to show up in the command palette. @mjbvz is that correct?

Copy link
Member

@amcasey amcasey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, though the enum structure feels forced.

@SudoPlz
Copy link

SudoPlz commented Feb 3, 2023

how do we use a specific mode in our settings.json ?

Are we supposed to do something like:

  "editor.codeActionsOnSave": {
    "source.sortImports": 'all',
  }

?

@andrewbranch
Copy link
Member Author

The command names are source.organizeImports, source.removeUnusedImports, and source.sortImports.

@SudoPlz
Copy link

SudoPlz commented Feb 3, 2023

thanks @andrewbranch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

organizeImports without the reordering ("remove unused imports" being its own option)
6 participants