lsp-scala icon indicating copy to clipboard operation
lsp-scala copied to clipboard

Move to MELPA or fold into lsp-clients

Open rossabaker opened this issue 7 years ago • 12 comments

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 avatar Dec 18 '18 02:12 rossabaker

The metals-specific functionality in this library are:

  • The four commands built on lsp-send-execute-command
  • Sending lsp-scala--config-options, which was useful in 0.2 but is empty now.

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.

rossabaker avatar Dec 18 '18 04:12 rossabaker

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).

strobe avatar Dec 18 '18 06:12 strobe

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'))

olafurpg avatar Dec 18 '18 06:12 olafurpg

@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 avatar Dec 18 '18 19:12 rossabaker

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 avatar Dec 18 '18 20:12 rossabaker

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

rossabaker avatar Dec 18 '18 21:12 rossabaker

@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.

strobe avatar Dec 19 '18 05:12 strobe

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

JesusMtnez avatar Dec 19 '18 06:12 JesusMtnez

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

strobe avatar Dec 19 '18 06:12 strobe

@strobe you're welcome, no problem.

JesusMtnez avatar Dec 19 '18 08:12 JesusMtnez

@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.

kurnevsky avatar Jun 04 '19 04:06 kurnevsky

@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.

rossabaker avatar Jun 04 '19 12:06 rossabaker