rossabaker / lsp-scala

Scala support for lsp-mode using metals
GNU General Public License v3.0
49 stars 6 forks source link

Move to MELPA or fold into lsp-clients #15

Open rossabaker opened 5 years ago

rossabaker commented 5 years ago

It's silly that we have to clone this instead of installing from a package manager. We should either:

  1. Justify our existence as a standalone project and submit to MELPA.
  2. PR ourselves to lsp-clients.el and dissolve this project.
rossabaker commented 5 years ago

The metals-specific functionality in this library are:

By staying standalone, we have Scala people doing the releases. We could add more metals specific commands without wondering whether we are still a "simple" client.

By merging into lsp-clients.el, we would have Elisp people doing the releases (i.e., we'd be free of fools like me.) Users would have one less package to configure. I'm leaning toward this being the better option.

Discuss.

strobe commented 5 years ago

Justify our existence as a standalone project and submit to MELPA.

A bad thing about merging it to lsp-clients.el is that we will lose eglot compatibility. Seems like for few peoples like me lsp-mode doesn't work for some reason but eglot going well (maybe it's doesn't work because of 'rootUri' detection bug or another build import issue).

olafurpg commented 5 years ago

For what it's worth, the vim installation instructions use vim-lsc directly without a custom Metals package https://scalameta.org/metals/docs/editors/vim.html. My impression is that this is working OK for people, at least no one has requested a vim-metals package. For commands, people write the raw JSON-RPC commands directly

:call lsc#server#call(&filetype, 'workspace/executeCommand', { 'command': 'build-import' }, function('abs'))
rossabaker commented 5 years ago

@strobe Does this package help with eglot at all? Trying eglot is also on my todo list, but I assumed that this package would be of no use in that world.

I've engaged the lsp-mode Gitter to get more opinions on the right place for this. Will report back.

rossabaker commented 5 years ago

The lsp-mode folks say they'd accept a PR, but that they lack expertise in the individual languages. They also mentioned that it's hard for people to install separate packages, but Metals has a really good docs story thus far. Finally, we'd be the first client there with extension methods. I think maybe a separate package in MELPA is the right place for this after all.

rossabaker commented 5 years ago

https://github.com/melpa/melpa/pull/5868

strobe commented 5 years ago

@strobe Does this package help with eglot at all? Trying eglot is also on my todo list, but I assumed that this package would be of no use in that world.

Yeah, my current setup which works looks like this https://gist.github.com/strobe/36f42d7867b0fb647f39d43aa2a9a4b4 and for some reason, if I try to change eglot to lsp-mode in that config then metals produce 'Skipping build import' messages and unable work properly.

But this solution with eglot is not clear because lsp-ui required files from lsp-mode therefore it should be installed anyway.

JesusMtnez commented 5 years ago

I tried eglot last week. You don't need lsp-scala at all to use eglot. The idea of eglot is to offer a common way to use lsp for all languagues, instead of having one definition per language as lsp-mode does. So you just need to install eglot and run M-x eglot in when you open a *.scala file

strobe commented 5 years ago

omg, you are right - I was completely wrong how that works together.

I was thought that lsp-ui required for navigation functions like lsp-ui-peek-find-definitions and lsp-scala needs to start metals via lsp-* package but seems like 'eglot' just works without anything else and has eglot--xref-reset-known-symbols with defaults keybindings.

sorry, for that

JesusMtnez commented 5 years ago

@strobe you're welcome, no problem.

kurnevsky commented 5 years ago

@rossabaker Time has passed and this package has become slightly outdated. Now all clients have list of defvars for custom settings. Also I think they don't mind of custom commands that use lsp-send-execute-command? So can this be reconsidered? Having this package as part of lsp-mode can help to keep it up-to-date. I can make a PR to lsp-mode myself if you don't have time for this.

rossabaker commented 5 years ago

@kurnevsky I think this is a good idea. It seems like more changes are being driven by lsp-mode than metals at this point, and it now makes a lot of sense to get into the mainline. I'd be in favor of sending a PR.