search

ycm-core / lsp-examples

Use any language server with YouCompleteMe.
https://ycm-core.github.io/YouCompleteMe
Apache License 2.0
82 stars 26 forks source link

feat: added example for Crystal language #26

Closed anton7c3 closed 3 years ago

anton7c3 commented 3 years ago

This change is Reviewable

bstaletic commented 3 years ago

Thanks for the pull request!

We usually add an install script that would, in this case, install crystalline somewhere in lsp-examples/crystal/. Would that be feasible?

anton7c3 commented 3 years ago

Thanks for the pull request!

We usually add an install script that would, in this case, install crystalline somewhere in lsp-examples/crystal/. Would that be feasible?

Sure, I think it's possible. EDIT: done.

anton7c3 commented 3 years ago

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @anton7c3)

crystal/install.py, line 86 at r2 (raw file):

try:
    shutil.move("/tmp/crystalline", "/usr/local/bin/crystalline")

Let's not write to /usr. Let's write it to lsp-examples/crystal/bin or something like that, plus a snippet.vim like the ones that exist for other languages. That would be a correct integration with the top level install.py.

And we don't really need the bash script. Windows normally doesn't have a bash shell.

There is a problem with Crystal for Windows. So far it is hardly usable, and no Crystalline version is available at all. Should I add some kind of stub for windows users?

anton7c3 commented 3 years ago

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @anton7c3)

crystal/install.py, line 86 at r2 (raw file): Previously, anton7c3 (Anton) wrote…

if OnWindows():
  sys.exit("Crystalline isn't supported on Windows")

Or something like that should be fine.

I've checked python script, it has an error message for all OSes that don't have Crystalline available, Windows included :) Everything should be fine now.

anton7c3 commented 3 years ago

Is there anything else I have missed?

bstaletic commented 3 years ago

I've tested the install script on my machine. Seems to be working. Merging.

bstaletic commented 3 years ago

@anton7c3 Once again, thanks for the pull request!