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

Add csharp lsp support #1788

Merged
merged 3 commits into from
Mar 12, 2022
Merged

Add csharp lsp support #1788

merged 3 commits into from
Mar 12, 2022

Conversation

Dispersia
Copy link
Contributor

OmniSharp is the defacto LSP for C#, so this just adds it as the default. Tested on Windows/Linux

@Philipp-M
Copy link
Contributor

I don't think the --hostPID is needed and it also needs the actual pid, which I don't think is added by helix?
I'm not sure about the rest of the parameters, my (working) configuration looks a little bit different.

This is my current language configuration (NixOS via home-manager)

{
  name = "c-sharp";
  language-server = { command = "omnisharp"; args = [ "-l" "Error" "--languageserver" "-z" ]; };
}

@Dispersia
Copy link
Contributor Author

Thanks for taking a look.
Stdio is a more feature-full version, as it allows two way communication compared to the http side:
https://github.com/OmniSharp/omnisharp-roslyn/blob/8f045ddc1922a48ddb9dbe2009e902d1c61453d2/src/OmniSharp.Host/CommandLineApplication.cs

however as for -l Error, I don't believe we would want to limit the logging output to at least less than Warning. OmniSharp will warn on things like package version collisions, things being restored as .net framework, etc, which are all warnings that you wouldn't get as just Error. I also went through the source for http/stdio/cli and couldn't find what the -z is for, do you have a link for the docs for it? https://github.com/OmniSharp/omnisharp-roslyn/blob/8f045ddc1922a48ddb9dbe2009e902d1c61453d2/src/OmniSharp.Host/CommandLineApplication.cs

and it seems you're right about hostPID, I had it auto fill in neovim, but it was working and I assume helix just closes the subprocess when it terminates, so I removed it.

Let me know what you think. Thanks!

@Philipp-M
Copy link
Contributor

Ok good to know, I agree with the logging output, let's keep it default.

Regarding -z:
https://github.com/OmniSharp/omnisharp-roslyn/blob/master/doc/Using-Omnisharp.md#one-based-indexes

@the-mikedavis
Copy link
Member

Could you commit the changes from cargo xtask docgen? That should fix the CI

@Dispersia
Copy link
Contributor Author

Haha, -z was even in the link I posted, i just completely glossed over it somehow.

And thanks, pushed the doc update!

Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

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

lgtm 🎉

@archseer archseer merged commit 0712eb3 into helix-editor:master Mar 12, 2022
the-mikedavis pushed a commit to the-mikedavis/helix that referenced this pull request Mar 12, 2022
* add csharp lsp support

* remove hostPID

* update docs
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.

5 participants