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

Send what features we support to the server. #279

Merged
merged 15 commits into from
Feb 28, 2018
Merged

Send what features we support to the server. #279

merged 15 commits into from
Feb 28, 2018

Conversation

LoneBoco
Copy link
Contributor

Some servers will listen to what features a client says it supports and will disable those features, returning "False" in the server capabilities reply. Which, in turn, disables that feature in our code.

Some servers will listen to what features a client says it supports and will disable those features, returning "False" in the server capabilities reply.  Which, in turn, disables that feature in our code.
Sometimes, a Windows pathname will be converted incorrectly.  This is a bandaid fix on the problem.

Previously, we would a path that started like this: \C:\Users\...

When we tossed that path into Sublime, it would not be able to find the view associated to that file.  This bandaid fix would remove the preceding \ in that pathname.

This will mainly fix issues with diagnostics.
@LoneBoco
Copy link
Contributor Author

The server in question where I encountered this issue was https://github.com/OmniSharp/omnisharp-roslyn

I'm not sure if sending everything in completionItemKind is correct or not. I figured I may as well.

According to the LSP specifications, the processId field in the initialization params needs to be the parent process id.  This is so the language server can shut itself down with the parent process terminates.  This will fix bugs where when you close Sublime Text, the language server would be kept running.
}
"signatureHelp": {
"signatureInformation": {
"documentationFormat": ["plaintext"]
Copy link
Member

Choose a reason for hiding this comment

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

I'm fairly certain you can set this to markdown.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added markdown to the list.

@rwols
Copy link
Member

rwols commented Feb 18, 2018

I’m eager to try this out tomorrow!

Copy link
Contributor

@tomv564 tomv564 left a comment

Choose a reason for hiding this comment

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

Thanks, this is a major catch-up on LSP's protocol version support.
LSP also supports markdown for hover, could you add that?

tomv564
tomv564 previously approved these changes Feb 27, 2018
@tomv564 tomv564 dismissed their stale review February 27, 2018 22:06

Spotted some unrelated changes

@tomv564
Copy link
Contributor

tomv564 commented Feb 27, 2018

Whoops, I see the URI fix is also in here, could you keep that in the other PR?

@LoneBoco
Copy link
Contributor Author

@tomv564 I thought I reverted my accidental merge. Is it still showing?

@tomv564 tomv564 merged commit a6ed42f into sublimelsp:master Feb 28, 2018
@tomv564
Copy link
Contributor

tomv564 commented Feb 28, 2018

It's likely I only saw the old commit and didn't check the diff - sorry to bother you.
Thanks for the initiative, and good to hear omnisharp being used with LSP!

@LoneBoco LoneBoco deleted the send-supported-features branch February 28, 2018 18:38
@rwols
Copy link
Member

rwols commented Mar 4, 2018

@LoneBoco do you happen to work with macOS? I tried several times to install the OmniSharp LSP server but never got it to work.

@LoneBoco
Copy link
Contributor Author

LoneBoco commented Mar 5, 2018

@rwols I do not. I use omnisharp-roslyn in Windows. According to the README.md, you will need mono (>=5.2.0) to run it on OSX. You should be able to clone it and run build.sh to have it download everything, build it, and run all the tests.

These are the settings I use with LSP:

{
    "clients":
    {
        "csharp":
        {
            "command": ["C:/Projects/omnisharp-roslyn/artifacts/scripts/OmniSharp.Stdio.cmd", "-lsp"],
            "scopes": ["source.cs"],
            "syntaxes": ["Packages/C#/C#.sublime-syntax"],
            "languageId": "csharp"
        }
    }
}

One thing to note is that omnisharp-roslyn has issues with the current master branch. Some of them are partially fixed, but haven't been merged back yet.

I've forked omnisharp-roslyn and pushed my own fixes for OmniSharp/omnisharp-roslyn#1113, OmniSharp/omnisharp-roslyn#1119, and OmniSharp/omnisharp-roslyn#1125. If you want the most problem free version of omnisharp-roslyn to mess with, I would recommend you clone from me for the time being.

Also, I would recommend you run with logging enabled. I had originally tried to open a project and it didn't work. The logs showed that msbuild was not able to process the solution as the project was using .NET 4.7 and I didn't have that version installed on my machine. That might be a reason why you are having issues.

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.

3 participants