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

Support plugins configuration in omnisharp.json #1615

Merged
merged 16 commits into from Nov 21, 2019

Conversation

@dmgonch
Copy link
Contributor

dmgonch commented Sep 29, 2019

Locations for OminSharp’s plugins can currently be configured only via –plugin command line argument. But when OminSharp is activated as a part of ms-vscode.csharp extension, other VSCode extensions cannot use this approach to supply their own plugins for OmniSharp. The only exception are code actions which can be configured in omnisharp.json via RoslynExtensionsOptions. The proposed changes enable OmniSharp to discover any plugin using the same technique via new Plugins section in omnisharp.json, i.e.:

"Plugins": {
    "LocationPaths": [ "path_to_plugin1.dll", "path_to_plugin2.dll" ]
}

An example of VSCode extension that provides plugins via omnisharp.json is Roslynator which does it via RoslynExtensionsOptions.

@dmgonch dmgonch marked this pull request as ready for review Sep 29, 2019
dmgonch and others added 3 commits Sep 29, 2019
…onch/oms-r into dmgonch/PluginsConfiguration
Dmitry Goncharenko
@filipw

This comment has been minimized.

Copy link
Member

filipw commented Oct 1, 2019

thanks - overall this is a good idea and a welcome addition.
I am not convinced that this is the best implementation though. I'd rather see types under OmniSharp.Options to be used in a consistent, strongly typed way rather than using the raw IConfigurationRoot to bind to them multiple times in multiple places.

This would mean that we need to refactor the code a bit to perhaps delay the decision about plugin discovery or move the options bootstrapping up.
I will have a closer look when I have some time.

@david-driscoll

This comment has been minimized.

Copy link
Member

david-driscoll commented Oct 2, 2019

I'm with @filipw I like the idea but the solution feels wrong somehow. I'm going to have to circle back to this, this weekend after my local code camp.

@dmgonch

This comment has been minimized.

Copy link
Contributor Author

dmgonch commented Oct 5, 2019

Good feedback! Switched to using strongly typed object for declaring locations for plugins.

@dmgonch

This comment has been minimized.

Copy link
Contributor Author

dmgonch commented Oct 10, 2019

@filipw @david-driscoll Curious what you guys think of the latest changes.

@filipw

This comment has been minimized.

Copy link
Member

filipw commented Oct 10, 2019

thanks a lot - it will be reviewed carefully as soon as there is some free time.
but after #1625 we don't want to rush any changes without thoroughly testing and pulling locally.

@dmgonch

This comment has been minimized.

Copy link
Contributor Author

dmgonch commented Nov 14, 2019

@filipw a gentle ping since it's been few weeks. thanks

@filipw

This comment has been minimized.

Copy link
Member

filipw commented Nov 14, 2019

sorry! I will do my best to review in the coming days! 🙏

@filipw
filipw approved these changes Nov 15, 2019
Copy link
Member

filipw left a comment

LGTM - thanks!

@filipw

This comment has been minimized.

Copy link
Member

filipw commented Nov 15, 2019

any thoughts? @david-driscoll @mholo65 @JoeRobich

if not IMHO this is good to merge and a very valuable feature

filipw added 2 commits Nov 15, 2019
Copy link
Member

mholo65 left a comment

Great work. A minor nit/question about the logging in the assembly loader extension.

Copy link
Collaborator

JoeRobich left a comment

LGTM

…ILogger"

This reverts commit 65899df.
@david-driscoll

This comment has been minimized.

Copy link
Member

david-driscoll commented Nov 20, 2019

LGTM

@filipw filipw merged commit a4e73f0 into OmniSharp:master Nov 21, 2019
6 checks passed
6 checks passed
OmniSharp.omnisharp-roslyn #1.34.8-PullRequest1615.34 succeeded
Details
OmniSharp.omnisharp-roslyn (GitVersion) GitVersion succeeded
Details
OmniSharp.omnisharp-roslyn (Linux) Linux succeeded
Details
OmniSharp.omnisharp-roslyn (Release) Release succeeded
Details
OmniSharp.omnisharp-roslyn (Windows) Windows succeeded
Details
OmniSharp.omnisharp-roslyn (macOS) macOS succeeded
Details
@filipw

This comment has been minimized.

Copy link
Member

filipw commented Nov 21, 2019

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.