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 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 September 29, 2019 20:15
@filipw
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
Copy link
Member

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
Copy link
Contributor Author

dmgonch commented Oct 5, 2019

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

@dmgonch
Copy link
Contributor Author

dmgonch commented Oct 10, 2019

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

@filipw
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
Copy link
Contributor Author

dmgonch commented Nov 14, 2019

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

@filipw
Copy link
Member

filipw commented Nov 14, 2019

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

Copy link
Member

@filipw filipw left a comment

Choose a reason for hiding this comment

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

LGTM - thanks!

@filipw
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

Copy link
Member

@bjorkstromm bjorkstromm left a comment

Choose a reason for hiding this comment

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

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

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

@david-driscoll
Copy link
Member

LGTM

@filipw filipw merged commit a4e73f0 into OmniSharp:master Nov 21, 2019
@filipw
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants