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

Add "Sort Imports" command to vscode extension & lsp #635

Merged
merged 6 commits into from
Nov 14, 2023

Conversation

camerondubas
Copy link
Contributor

As requested here #626, this adds a "Sort Imports" command to the vscode extension and implements a request in the language server to get the TextEdits needed to organize the imports of the current file.

  • works with gts files as well as ts
  • currently set to be non-destructive, so unused imports only reordered, not removed. Wasn't sure what the standard should be for this, or if it should be configurable by the user.
Recording.2023-10-16.214222.mp4

let preferences = {};
let edits = server.organizeImports(project.fileURI('index.ts'), formatting, preferences);

expect(edits).toEqual([
Copy link
Contributor

Choose a reason for hiding this comment

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

why are their edits if the imports are already sorted?

Copy link
Contributor

Choose a reason for hiding this comment

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

const language = server.getLanguageType(uri);
const formatting = configManager.getFormatCodeSettingsFor(language);
const preferences = configManager.getUserSettingsFor(language);
return server.organizeImports(uri, formatting, preferences);
Copy link
Contributor

Choose a reason for hiding this comment

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

🥳

Copy link
Contributor

@NullVoxPopuli NullVoxPopuli left a comment

Choose a reason for hiding this comment

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

thanks for doing this! it's exciting to see more features be added!

@dfreeman dfreeman added the enhancement New feature or request label Oct 17, 2023
Copy link
Member

@dfreeman dfreeman left a comment

Choose a reason for hiding this comment

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

Thank you so much for taking this on, @camerondubas! It's super exciting to see new contributors digging in and making hefty contributions like this 🙂

ProtocolRequestType<RequestParams, TextEdit[] | null, void, void, void>
);

export interface RequestParams {
Copy link
Member

Choose a reason for hiding this comment

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

It's arguably incidental that getIR and sortImports both just want a uri param, while other requests might require more (or less!) information. Given that, let's keep separate xParams/xResult types for each message.

packages/vscode/src/extension.ts Outdated Show resolved Hide resolved
packages/vscode/package.json Show resolved Hide resolved
packages/core/__tests__/language-server/requests.test.ts Outdated Show resolved Hide resolved
packages/core/__tests__/language-server/requests.test.ts Outdated Show resolved Hide resolved
@ijlee2
Copy link

ijlee2 commented Oct 17, 2023

How does the Sort Imports command work on a project that already has eslint-plugin-simple-import-sort (this plugin works on *.{gjs,gts} files)? Would there be some conflict between the two?

@dfreeman
Copy link
Member

@ijlee2 I imagine it would interact in exactly the same way as that plugin does with the built-in "Sort Imports" commands that are provided for regular JS and TS files today. Presumably if you're using the eslint plugin, then you aren't invoking this command and instead are relying on eslint --fix or fix-on-save instead.

@ijlee2
Copy link

ijlee2 commented Oct 17, 2023

@dfreeman Sounds good, thanks for the clarification.

@camerondubas camerondubas requested a review from dfreeman October 22, 2023 14:04
@NullVoxPopuli
Copy link
Contributor

@camerondubas looks like there are lint failures? Can you address and push?
Thanks!

@dfreeman dfreeman force-pushed the feature/sort-imports branch from d910b61 to 2f1c5b9 Compare November 14, 2023 15:50
@dfreeman dfreeman force-pushed the feature/sort-imports branch from 2f1c5b9 to 52a51d6 Compare November 14, 2023 15:53
Copy link
Member

@dfreeman dfreeman left a comment

Choose a reason for hiding this comment

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

I hope you don't mind I pushed the lint fixes myself @camerondubas! I have a bit of time this week and wanted to make sure this made it into the release I'll be putting out 🙂

We have a test issue right now that only occurs with the TS nightly build and is only reproducible in CI (💀), but that has nothing to do with your change here, so I think we're good to go. Thank you so much! 🎉

@dfreeman dfreeman merged commit 44491c9 into typed-ember:main Nov 14, 2023
3 of 4 checks passed
@camerondubas
Copy link
Contributor Author

Thanks for getting this in!

@bartocc
Copy link

bartocc commented Nov 27, 2023

Thx for this feature 🙏

We've been using source.organizeImports for a while, and this both sorts, but also removes unused imports.
@camerondubas , I know you chose to not implement this feature in this PR, but are there any plans on adding this?

@camerondubas
Copy link
Contributor Author

@bartocc Ya I could take a look at that. Should have some time over the next week

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants