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

Remove LowerCaseDriveLetter function #207

Merged
merged 1 commit into from
Aug 29, 2015
Merged

Remove LowerCaseDriveLetter function #207

merged 1 commit into from
Aug 29, 2015

Conversation

micbou
Copy link
Contributor

@micbou micbou commented Aug 28, 2015

This PR removes the use of the LowerCaseDriveLetter function. There are several issues with it:

  • its purpose is to fix an issue with Vim. Ideally, client-specific issue should be fixed in the client, not in the server ;
  • it forces clients and tests framework to apply the same hack when interacting with the server. See this commit as an example ;
  • it seems unnecessary. All tests pass on Windows without it.
  • it only lowers the C and D drive letters. What if another letter is used?

@nosami
Copy link
Contributor

nosami commented Aug 29, 2015

Yes. You're absolutely right. This was a dirty hack that should never have been here.

btw. This project is obsolete. This issue doesn't exist in omnisharp-roslyn.

nosami added a commit that referenced this pull request Aug 29, 2015
Remove LowerCaseDriveLetter function
@nosami nosami merged commit e190291 into OmniSharp:master Aug 29, 2015
@Valloric
Copy link

btw. This project is obsolete. This issue doesn't exist in omnisharp-roslyn.

Whoa, that's news to me. :) Should ycmd migrate to omnisharp-roslyn? Is that ready for prime-time?

homu added a commit to ycm-core/ycmd that referenced this pull request Aug 30, 2015
[Windows support] Update OmniSharpServer submodule

OmniSharp Server was modifying the drive letter case of paths causing test failures on ycmd side. This is fixed upstream (see PR OmniSharp/omnisharp-server#207) so we need to update the submodule.

Fix 6 tests (1 error and 5 failures) on Windows (listed in the commit message).
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