-
Notifications
You must be signed in to change notification settings - Fork 27
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
feat(lsp): support using new language server implementations #162
base: main
Are you sure you want to change the base?
Conversation
|
Let's make this draft while it is still WIP. |
"enum": ["extension", "nu --lsp", "nuls"], | ||
"default": "extension", | ||
"description": "Switch between different language server implementations (requires restarting Visual Studio Code to take effect)." | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bah, this is the only deliberate change here
The rest is auto-prettier
, which I can revert if necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're using vscode to edit these files, one thing that I've just learned is you can do ctrl/cmd-shift-p and choose File:Save without formatting
and it stops things like prettier or other formatters from running on save.
@fdncred I assume those are notes on |
Yes, those are notes on I think there should be some type of restart on this PR when the lsp changes. |
Okay, I've made "implementation" a live setting It's working suspiciously well, haha |
I will say though, the switching between |
The current behaviour tries to stop all the servers and then start the desired one, and maybe those errors are from trying to stop a server that was never running Just a theory, but I'll play with it and see Thanks for checking! |
I'm handling the Promises from That said, I believe them to be pretty harmless, and you probably won't encounter them at all unless you try to use an implementation that cannot be started, i.e. you don't have |
Thanks! I'll give this a try later this afternoon. Just so you know, I had |
Sorry - Ran out of time today. I hope to look at this tomorrow. |
No worries Not urgent And I've had less time to work in this area in the last week, too :) |
@fdncred, thanks for the feedback and great to see that it is also working in Vscode (I'm using Neovim).
I observed similar behavior in my editor of choice and that it seems to work only in certain conditions. Here is an example of my setup. I type something into the command line, start Nushell's open in editor feature, then my Neovim starts and start the LSP automatically, I can use some completions and then I quit the editor: However, when the document changed completions stop working unless I was the document. And here the |
@fdncred, speaking of “to show”: if you want I could provide some GIF recording of Nushell in Neovim if you want to include it in the release blog post. |
ya, definitely. Feel free to put them in this PR nushell/nushell.github.io#1114 with a write up like "We've started building a nushell Language Server". What most people are looking for at this point is (1) how to configure it in helix, vim, nvim and (2) which features are currently supported. |
@schrieveslaach |
@amtoine, at the moment it is very simple. I start an LSP for the current buffer when file type nu. function StartNuLS()
lua << EOF
local id = vim.lsp.start({
name = "Nushell's LSP",
cmd = {
"/pat/to/nushell-repo/target/debug/nu",
"--lsp"
},
on_attach = M.on_attach
})
vim.lsp.buf_attach_client(0, id)
EOF
endfunction
augroup NuSetup
autocmd!
autocmd FileType nu call StartNuLS()
augroup end I was thinking of providing a PR to lspconfig after Nushell's next release which does it then for you. Maybe I'll also try to add file type nu to Neovim directly. |
@schrieveslaach |
This PR only needs to support 2 lsps now. |
We probably want to determine what the destiny of this PR is I agree we should remove the But is it desirable to ship/merge this PR otherwise (assuming other rough areas are polished)? Or, should we keep this as a PR until |
I'd like to have the functionality of switching between the two lsps until nu-lsp overtakes nu --ide-* in functionality and stability. This PR isn't far from landing. We just need to work on the things mentioned, logging, making sure to default to nu --ide-*, etc. |
@fdncred Oh, my bad, sorry if I was disrespectful. I lost something in the translation 😅 My understanding is that the problems mentioned in this PR are solved in other PRs (either merged, or ready to merge when the moment comes)
and, the remaining issues:
I don't know how to reproduce the problem, can you give me a little script or sample to follow? 💯 . That is a note for
I had some of those errors too, but it was something of VS Code playing badly with the extension. I decided to try the extension host with all the extensions disabled and it didn't failure once. Still have that weird bug that sometimes the LSP has more than one instance running And that's about it. I see nothing else to be missing. That's why I asked before for "what could be done to land this PR?" (in a poor manner) |
I think input for @AucaCoyan last comment would be also input for me on what I could work next in the LSP area. I'm a bit lost there at the moment because I just using it to edit my Nushell REPL input and don't often work with a lot of scripts. |
Looking at this PR, there are a few things that might need to change before landing. I've tried to re-read everything and add a checkbox for it below just to make sure we've covered everything. Some of these items may be done and we can just check them off. Others will need some work prior to landing this PR.
|
Thank you very much for creating a list with everything! 🙏🏼 I'm wondering on how to tackle the list. I think we should resolve changing servers and debugging + errors first. But since this is a PR, only jokeyrhyme and maintainers of this repo can make changes. Is it ok to you to make PRs into your repo |
or @AucaCoyan we can close this PR and you can copy the code out of it and make a new PR with you being the owner. i'm not sure if there's a way to give you permissions to change this PR without going through jokeyrhyme's repo. |
I've invited everyone here to collaborate on my fork, but have no ill feelings if you wish to fork my fork into another repository :) |
Thanks @jokeyrhyme, we appreciate your help! |
Thank you jokey! I tested it and updated main of your repo successfully 😄 We'll be seeing around each other here and in the fork 💪🏼 |
remove `nuls` from the list of servers
nu --lsp
, orlsp
nu --ide-...
implementation embedded in this extensionnu --lsp
will use this implementation (if available): Integrated Language Server nushell#10723nuls
will use the implementation in this repository (if available): https://github.com/jokeyrhyme/nuls