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

Organise imports should be a refactoring/code action #46647

Closed
jrieken opened this issue Mar 27, 2018 · 7 comments · Fixed by #47850
Closed

Organise imports should be a refactoring/code action #46647

jrieken opened this issue Mar 27, 2018 · 7 comments · Fixed by #47850
Assignees
Labels
api editor-code-actions Editor inplace actions (Ctrl + .) feature-request Request for new features or functionality on-testplan upstream Issue identified as 'upstream' component related (exists outside of VS Code)
Milestone

Comments

@jrieken
Copy link
Member

jrieken commented Mar 27, 2018

re #46589

After the investment in code actions, the refactoring menu, and code action keybindings I was quite surprised to see that organise imports isn't a code action but a normal command. I think that makes it harder to discover and also encourages homegrown code action instead of the more stream lined approach.

@mjbvz
Copy link
Collaborator

mjbvz commented Mar 27, 2018

This was my initial proposal too. However I now think that command makes more sense as organize imports is a file wide action (and will eventually be triggered on save)

However we could also surface this as a code action to improve discoverability. I've opened microsoft/TypeScript#22909 to track this

@mjbvz mjbvz closed this as completed Mar 27, 2018
@mjbvz mjbvz added feature-request Request for new features or functionality upstream Issue identified as 'upstream' component related (exists outside of VS Code) typescript Typescript support issues javascript JavaScript support issues labels Mar 27, 2018
@jrieken
Copy link
Member Author

jrieken commented Mar 28, 2018

makes more sense as organize imports is a file wide action (and will eventually be triggered on save)

You mean like format document ;-) ? I disagree with 'upstream'-labelling because it is us exposing the functionality in VS Code and us to decide how that happens - as code action or command.

The approach taken is wrong and reminds me of the formatting-via-command-story (not as provider) for which we went great lengths to educate extension authors. Now we set the same bad sample and even today things aren't working properly: in my install I have the Java extension which uses the same anti-pattern (mostly likely because code action weren't ready yet) and the assigned keybinding always runs the Java-command, never the TypeScript command...

screen shot 2018-03-28 at 09 26 07

@jrieken jrieken reopened this Mar 28, 2018
@jrieken
Copy link
Member Author

jrieken commented Mar 28, 2018

formatting-via-command-story (not as provider) for which we went great lengths to educate extension authors.

For reference - we blogged about this and tracked down every single formatter-command-extension with issues/PRs

@mjbvz
Copy link
Collaborator

mjbvz commented Mar 28, 2018

I’m still not convinced that code actions are the right ux. The intended usage of organize imports is very much like format in that it is independent of cursor position and run on the entire current document. And format—whether implemented using providers or actual commands—is a command from the user’s perspective.

I’ve already submitted a pr against java for this. I agree that having an organize imports command per lanague is not scalable but think we should look at other approaches, such as a new provider type, if this ends up being a problem

@jrieken
Copy link
Member Author

jrieken commented Mar 28, 2018

I’m still not convinced that code actions are the right ux. The intended usage of organize imports is very much like format

Well, it will allow us to have something like Context Menu > Refactor > Organise Imports. I'd say that makes the feature way more discoverable than the command palette with duplicate entries in which I need to scan language-prefixes (that hopefully exist, everybody could just call things 'Organise Imports').

And format—whether implemented using providers or actual commands—is a command from the user’s perspective.

Yes - everything is a command even IntelliSense but the point of well-know-thanks-to-api-commands is better UX integration. Now we have picked the worst in the face of a generic code actions infrastructure that would allow for better UI. What don't we add a new code action kind which about 'sugar', e.g. Format, Sort Members, Organise Imports, Align Whitespaces, Insert Copyright Header, etc.?

@johnnyreilly
Copy link

My issue was closed as a duplicate of this. I don't believe it is but it is related. To keep everything in one place I'll paste my specific issue here so it doesn't get lost:

The "Remove Unnecessary Usings" lightbulb for C# is super cool. Love it.

However, I have roll over the unnecessary using before I can see that I actually have unnecessary usings. It would be great if:

  • Unnecessary usings were more obvious.
  • That I was prompted ahead of actually moving over the unnecessary usings.

Love your work by the way - loving coding C# in VS Code

@jrieken jrieken added api editor-code-actions Editor inplace actions (Ctrl + .) and removed javascript JavaScript support issues typescript Typescript support issues labels Apr 9, 2018
@jrieken
Copy link
Member Author

jrieken commented Apr 9, 2018

@johnnyreilly The lightbulb is controlled by the Omnisharp/C#-extension, so ultimately that must change. This issue is about show-casing how organise imports et all can be integrated into VS Code best. E.g. similar to how it is done in Eclipse (have a source and refactor menu)

screen shot 2018-03-28 at 12 05 31

@mjbvz mjbvz added this to the April 2018 milestone Apr 16, 2018
mjbvz added a commit to mjbvz/vscode that referenced this issue Apr 17, 2018
Fixes microsoft#47845
Fixes microsoft#46647

- Defines a new standard `SourceOrganizeImports` `CodeActionKind` to be used to implement organize imports in a consistent way.
- Add a new `Organize imports` command and keybinding that executes these actions.
- Move over the existing js/ts organize imports command to use the new code action kind
mjbvz added a commit that referenced this issue Apr 18, 2018
)

* Move TS/JS to use organize imports code action

Fixes #47845
Fixes #46647

- Defines a new standard `SourceOrganizeImports` `CodeActionKind` to be used to implement organize imports in a consistent way.
- Add a new `Organize imports` command and keybinding that executes these actions.
- Move over the existing js/ts organize imports command to use the new code action kind

* Use supportedCodeActions context key

* Document code action kind values

* Fix regular expression

Make sure we only match whole scopes and not `unicorn.source.organizeImports`
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api editor-code-actions Editor inplace actions (Ctrl + .) feature-request Request for new features or functionality on-testplan upstream Issue identified as 'upstream' component related (exists outside of VS Code)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants