Skip to content

Change "Organize Imports" command label to "Optimize Imports" #232869

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.

Sign up for GitHub

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

Jump to bottom
Merged
mjbvz merged 3 commits into from
Nov 6, 2024
Merged

Change "Organize Imports" command label to "Optimize Imports" #232869

mjbvz merged 3 commits into from
Nov 6, 2024

Conversation

@Kannav02
Copy link
Contributor

@Kannav02 Kannav02 commented Nov 1, 2024

This PR fixes Issue #232351

Changes

  • Updated the command label from "Organize Imports" to "Optimize Imports" in the editor action
  • Updated the title in TypeScript language features extension

Modified Files

  • src/vs/editor/contrib/codeAction/browser/codeActionCommands.ts
  • extensions/typescript-language-features/src/languageFeatures/organizeImports.ts

Testing

I have used my own typescript project to test out this feature

  • Verified command appears as "Optimize Imports" in:
    • Command Palette
Screenshot 2024-11-01 at 5 56 47 PM
  • Context Menu
  • Keyboard Shortcut (Shift+Alt+O) tooltip
Screenshot 2024-11-01 at 5 57 07 PM

Note

The command ID organizeImportsCommandId remains unchanged to maintain compatibility and also I have not changed much of the meta-data associated with the same as I didn't find the need to, if you do want me to change it please do let me know

@Kannav02
Copy link
Contributor Author

Kannav02 commented Nov 1, 2024

@microsoft-github-policy-service agree

@gjsjohnmurray
Copy link
Contributor

gjsjohnmurray commented Nov 2, 2024

Won't this break things for users who are accustomed to typing "organize" in Command Palette?

Did consider the second suggestion in #232351 (comment) from @TylerLeonhardt?

Philosophical question. Does the command improve performance of the code, which is what the word "optimize" implies to me? Or does it just aid comprehension by the programmer?

@Kannav02
Copy link
Contributor Author

Kannav02 commented Nov 3, 2024

I believe this change is only made to aid the user with comprehension and to have consistency with other IDEs as mentioned in the issue

I did take a look at Tyler's comments

To include more meta-data information,I would have to make a substantial amount of changes, so before moving forward with this I wanted to have an opinion from the maintainers for the same, if they give me the green light, I'll go ahead and make the change.

Thank you!

mjbvz
mjbvz requested changes Nov 5, 2024
View reviewed changes
Copy link
Collaborator

@mjbvz mjbvz left a comment

Choose a reason for hiding this comment

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

I don't think we should change the names. Instead see the recommendation about adding metadata in the original issue: #232351 (comment)

@Kannav02
Copy link
Contributor Author

Kannav02 commented Nov 5, 2024

sure, I will make these changes

Thank you!

@Kannav02
Copy link
Contributor Author

Kannav02 commented Nov 6, 2024

The changes improve:

  • Command discovery through VS Code's command palette
  • Support for both "Organize" and "Optimize" terminology in searches, plus including other terminologies like "Sort Imports", "Unused Imports" and "Combining Imports"
  • TF-IDF scoring for related search terms
  • Also added the support for the type of inputs the mode argument would take to organize imports

Testing

  1. Command Palette Search:
    • Search for "organize imports"
    • Search for "optimize imports"
    • Search for "sort imports"
    • Search for "clean up imports"
    • Verify command appears in results

Notes

All of the changes made were purely meta-data changes and shouldn't impact the functionality of the function in any way possible except assisting in better TF-IDF scoring for the command's terms

@Kannav02 Kannav02 requested a review from mjbvz November 6, 2024 07:03
mjbvz
mjbvz reviewed Nov 6, 2024
View reviewed changes
Copy link
Collaborator

@mjbvz mjbvz 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 following up. Let's see if we can just add to the metadata description

@Kannav02
Copy link
Contributor Author

Kannav02 commented Nov 6, 2024

Sure, just to follow up , should i use nls.localize for the description in meta data since this command is being used in different languages?

Thank you!

@mjbvz
Copy link
Collaborator

mjbvz commented Nov 6, 2024

Yes you should use localize2 for that

@Kannav02
Copy link
Contributor Author

Kannav02 commented Nov 6, 2024

I have implemented the changes in the codebase, please do let me know if any other changes are required

Thank you!

mjbvz
mjbvz reviewed Nov 6, 2024
View reviewed changes
mjbvz
mjbvz approved these changes Nov 6, 2024
View reviewed changes
Copy link
Collaborator

@mjbvz mjbvz left a comment

Choose a reason for hiding this comment

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

Thanks!

@mjbvz mjbvz enabled auto-merge (squash) November 6, 2024 21:38
@vs-code-engineering vs-code-engineering bot added this to the November 2024 milestone Nov 6, 2024
@mjbvz mjbvz merged commit 1643da0 into microsoft:main Nov 6, 2024
@mjbvz mjbvz mentioned this pull request Dec 10, 2024
Closed
@vs-code-engineering vs-code-engineering bot locked and limited conversation to collaborators Dec 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@connor4312 connor4312 connor4312 approved these changes

@mjbvz mjbvz mjbvz approved these changes

Assignees

@mjbvz mjbvz

Labels

None yet

Projects

None yet

Milestone

November 2024

Development

Successfully merging this pull request may close these issues.

4 participants

@Kannav02 @gjsjohnmurray @mjbvz @connor4312