Correctly set formatting options on the workspace #770
Conversation
|
At least once CI passes... :) |
| { | ||
| try | ||
| { | ||
| Workspace.Options = workspaceOptionsProvider.Process(Workspace.Options); |
There was a problem hiding this comment.
This implies an order to the IWorkspaceOptionsProviders. Is there a way we can define that?
There was a problem hiding this comment.
I wanted to avoid getting into that yet, because that would need a more generic solution anyway - for the plugins.
I believe the best approach would be to use MEF metadata something like:
public interface IPlugin
{
string Name { get; }
int Order { get; }
}
[MetadataAttribute]
public class PluginMetadataAttribute : ExportAttribute, IPluginMetadata
{
public string Name { get; set; }
public int Order { get; set; }
public PluginMetadataAttribute()
: base(typeof(IPlugin))
{
}
}
[PluginMetadata(Name = "DefaultCSharpFormatting", Order = -1)]
And then use this metadata when plugins are discovered; this would apply to all plugins in the future. However, I believe we would need to design it a bit more carefully - I'll open a separate issue to track that
src/OmniSharp.Host/Startup.cs
Outdated
| } | ||
| catch (Exception e) | ||
| { | ||
| var message = $"The workspace optiosn provider '{workspaceOptionsProvider.GetType().Name}' threw exception during initialization."; |
There was a problem hiding this comment.
Might be helpful to see the workspaceOptionsProvider.GetType().FullName here.
| @@ -69,6 +71,10 @@ protected Task<OmniSharpWorkspace> CreateWorkspaceAsync(params TestFile[] testFi | |||
|
|
|||
| var workspace = plugInHost.GetExport<OmniSharpWorkspace>(); | |||
|
|
|||
| // OmniSharp ships only one provider, CSharpWorkspaceOptionsProvider | |||
| var formattingProvider = plugInHost.GetExports<IWorkspaceOptionsProvider>().FirstOrDefault() as CSharpWorkspaceOptionsProvider; | |||
There was a problem hiding this comment.
Given the comment, would Single() be a better choice for tests?
| @@ -69,6 +71,10 @@ protected Task<OmniSharpWorkspace> CreateWorkspaceAsync(params TestFile[] testFi | |||
|
|
|||
| var workspace = plugInHost.GetExport<OmniSharpWorkspace>(); | |||
|
|
|||
| // OmniSharp ships only one provider, CSharpWorkspaceOptionsProvider | |||
| var formattingProvider = plugInHost.GetExports<IWorkspaceOptionsProvider>().FirstOrDefault() as CSharpWorkspaceOptionsProvider; | |||
| workspace.Options = formattingProvider?.Process(workspace.Options); | |||
There was a problem hiding this comment.
As written, if the CSharpWorkspaceOptionsProvider isn't included in the MEF composition, this will set workspace.Options to null. Intentional?
| { | ||
| public interface IWorkspaceOptionsProvider | ||
| { | ||
| OptionSet Process(OptionSet workOptionSet); |
There was a problem hiding this comment.
I did a bit of a double-take when I read workOptionSet. I wasn't sure if this meant "working option set" or "workspace option set". Maybe just optionSet to avoid confusion?
|
All of the CI failed with It looks like there was an update to xunit.runner.console. I bet that broke it. |
|
Yup. performing a |
|
PR is out to fix build: #772. |
|
There we go. 😄 |
thanks! I didn't notice it - it worked when I ran the tests from VS 👍 |
filipw
force-pushed
the
feature/workspace-formatting
branch
from
5df32d0 to
26b7e72
Compare
thanks for the feedback - I pushed the smaller things into this branch and spun off #773 specifically as part of the plugin infrastructure |
|
|
Tests run for you in VS? I haven't been able to get tests to run in VS since moving OmniSharp to netcoreapp1.1. |
|
They do, but only when
|
|
Ahhhh.... We should change that then. |
fixes #759
This PR changes our approach to CSharp formatting options. So far, we'd use the OmniSharp workspace without paying any attention to formatting settings - those were always applied on demand via each of the formatting endpoints. As a result, any formatting change that happened differently than through those endpoints (i.e. running code actions) would not respect the settings.
The change here is to introduce a discoverable provider,
IWorkspaceOptionsProvider(maybe anyone can suggest a better name?) which is discovered at app startup and provides the workspace with formatting options. In our case, the only provider at the moment would beCSharpWorkspaceOptionsProviderbut in the future we could easily add more, or have different ones loaded via the plugin infrastructure, so from that perspective we are also introducing a new extensibility point into OmniSharp.