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

Omnisharp Roslyn support #287

Closed
wants to merge 24 commits into from
Closed

Conversation

alistair
Copy link

@alistair alistair commented Jan 1, 2016

This pull request contains initial support for using omnisharp-rosyln instead of the now legacy OmnisharpServer repository.

While most functionality seems to have been retained there is currently a regression in the YcmCompleter FixIt command. This is expecting a response but the omnisharp-rosyln server isn't returning one. I have yet to figure out whether this is expected or not.

There are 2 new commands which should be implemented /getcodeactions and /runcodeaction

examples of these requests and their response can be seen here https://gist.github.com/alistair/73bbd908b6464066e93d#file-getcodeaction

Also not that while I have attempted to make this work on windows, I haven't tested whether it actually does.

Feedback on this pull request would be welcome.

Review on Reviewable

@alistair alistair changed the title Omnisharp Rosyln support Omnisharp Roslyn support Jan 1, 2016
@alistair
Copy link
Author

alistair commented Jan 1, 2016

Ok, i'm hoping this will actually go green now (at least on linux). There are a number of things which need to be looked at and evaluated in terms of impact.

@alistair
Copy link
Author

alistair commented Jan 1, 2016

i'm suspecting that omnisharp-roslyn is taking too long to startup on travis. but will have to investigate later. far too late now.

@vheon
Copy link
Contributor

vheon commented Jan 1, 2016

We already had a PR for OmnisharpServer Roslyn #213. Did you look at it?

@Valloric
Copy link
Member

Valloric commented Jan 1, 2016

Thanks for the PR! :)

Like @vheon said, we've had a PR that tried to update to Omnisharp-roslyn so it's worth checking that out.

For the tests, I see that some are being marked as skipped. That's fine while you're developing, but of course those tests can't be skipped in the final code that will be merged. We'd all be happier with the roslyn version of omnisharp, but it can't introduce regressions.

For new subcommands, I'd suggest focusing this PR on replacing the functionality we have with the new omnisharp and leave exposing new features to a future PR.


Review status: 0 of 10 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


.gitmodules, line 28 [r1] (raw file):
Shouldn't the other omnisharp submodule be removed if this is being added?


ycmd/completers/cs/cs_completer.py, line 599 [r1] (raw file):
A better name might be PostToServer


ycmd/tests/cs/get_completions_test.py, line 114 [r1] (raw file):
Why are these tests being skipped?

I'm assuming this is temporary and the idea is to resolve this before the PR is declared ready to merge, correct?


Comments from the review on Reviewable.io

@mispencer
Copy link
Contributor

As far as I know, none of the issues with Omnisharp-rosyln mentioned in my earlier PR have been addressed, . It still lacks support for import types, hence those skip tests in this PR as well as in my original PR. This is a crucial feature to me, so I still can't support switching to Omnisharp-rosyln permanently. It also still has the Recommender issue open, so I'm assuming that is also an issue.

Last time, the performance of the HTTP interface was lacking, so I had to implement the STDIO interface. This quickly became a whirlpool of pain, so if the performance of HTTP has improved, I would prefer to use that for MUCH simpler interface.


Comments from the review on Reviewable.io

@alistair
Copy link
Author

alistair commented Jan 2, 2016

I really thought I looked whether someone had started work on rosyln before I started....
Oh well, I will chalk it up to experience learning ycmd internals. But yes the skipped tests were temporary :)
The other pull request looks further along so im going to close this. If you need any help on the other one, just ping me. more than interested to help :)

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.

4 participants