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

Added support for organizing imports in formatting #1686

Merged
merged 4 commits into from
Jan 24, 2020

Conversation

filipw
Copy link
Member

@filipw filipw commented Jan 22, 2020

The organize imports formatting feature is now public in Roslyn.
Added as a flag since we have an approach to make all these things configurable and opt-in.

@JoeRobich
Copy link
Member

Would it be good to have this functionality split the way VS does, Format vs Code Cleanup? The format command defaulting to just being whitespace formatting. Then a new code cleanup command which brings in other types of changes such as organize imports.

image

If the user always wanted the code cleanup behavior, then maybe there is an omnisharp option to run code cleanup during format.

@filipw
Copy link
Member Author

filipw commented Jan 22, 2020

yes we could have another endpoint for this and then build it up with other capabilities to fix i.e. the IDExxxx diagnostics and so on.

the reason why I added it to formatting endpoint here is that in LSP there is only the "formatting" concept, no "cleanup" concept, so I thought maybe that would be cleaner to not create something completely new.

Copy link
Member

@JoeRobich JoeRobich left a comment

Choose a reason for hiding this comment

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

LGTM

@filipw
Copy link
Member Author

filipw commented Jan 23, 2020

@JoeRobich thanks for the approve. I really think it makes sense this way.

in the "clean up" endpoint we could implement a more sophisticated handling like the current fix usings does already where it also removes unused stuff. It really also aligns with the feature discussed here #1581

by the way, do you currently call/plan to call this Formatter.OrganizeImportsAsync in dotnet format?

@JoeRobich
Copy link
Member

do you currently call/plan to call this Formatter.OrganizeImportsAsync in dotnet format?

We do. We plan to make it part of the "style" formatting which would also include running analyzers and code fixes. This work is hinging on getting changes into Roslyn for it to setup workspaces to automatically make use of the compiler's EditorConfig support and for Analyzers to pull their configuration Options from the AnalyzerConfigOptions instead of relying on a Workspace OptionSet.

@filipw
Copy link
Member Author

filipw commented Jan 23, 2020

OK very cool, then we will do the same in the future 😃

we need to change EditorConfig implementation to get support for dotnet_diagnostic severity control too.

@filipw filipw merged commit f19bb41 into master Jan 24, 2020
@filipw filipw deleted the feature/organize-imports-format branch January 24, 2020 12:19
filipw added a commit that referenced this pull request Jan 24, 2020
@filipw filipw mentioned this pull request Jan 24, 2020
bjorkstromm added a commit that referenced this pull request Jan 25, 2020
@jmihalich
Copy link

jmihalich commented Feb 4, 2020

Hi,

Is this actually live in 1.21.10 as it says in the release notes? Is there documentation for this feature anywhere, cuz i have a few questions:

  1. looking at the code change it looks like the relevant setting in omnisharp.json is FormattingOptions.OrganizeImports, and that's a boolean value?

  2. the auto fill is not showing an OrganizeImports option when i try to add it to the json file

  3. is it possible to specify the order of organization? or is it fixed based on an internal sort algorithm?

  4. is there lightbulb support for if you select or hover over a using statement?

Thanks,
Joe

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

Successfully merging this pull request may close these issues.

None yet

4 participants