Implement LSP Text Document Synchronization #10941
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@fdncred, improve completions are working now better. 😉 |
|
whoa! that looks great! looks like clippy is objecting to something, as usual :) and i love all the tests! keep up the great work! |
fdncred
reviewed
Nov 3, 2023
fdncred
reviewed
Nov 3, 2023
fdncred
reviewed
Nov 3, 2023
schrieveslaach
force-pushed
the
lsp-document-synchronization
branch
2 times, most recently
from
ca21211
to
cb76c41
Compare
|
I'm seeing weird stuff in helix. I can't get hovers to work, and completions only seem to work when it's in the root of a script and not inside of a def. After about 20 seconds waiting for a hover, this appears. and when I exit it takes a while and then it finally exits with this message. |
|
Hmmm, it's not really working in vscode either. goto-definition isn't working, nor is hover. |
schrieveslaach
force-pushed
the
lsp-document-synchronization
branch
from
cb76c41
to
5989ccc
Compare
|
@fdncred, are these errors popping up with this PR or are they are also popping up on main? |
|
@schrieveslaach They're specifically with this PR. |
schrieveslaach
force-pushed
the
lsp-document-synchronization
branch
from
5989ccc
to
93011b8
Compare
|
@fdncred, could you give it another try? |
|
I'm still getting this on the latest version. I wish I knew how to better support you. |
|
On the server side, I got this. Just saw it. |
schrieveslaach
force-pushed
the
lsp-document-synchronization
branch
2 times, most recently
from
fe9a15b
to
590f2f5
Compare
|
That was a good hint and made me look into the spec:
So, in case that there is no result, param_handler(engine_state, ¶ms)
.and_then(|response| serde_json::to_value(response).ok())
.unwrap_or(serde_json::Value::Null), |
|
Looking better. Still need markdown formatting but at least something is popping up now. But variables are being duplicated somewhere.
goto-definition seems to be working great I can't really tell about completions. I guess they're working. I think the implementation of the vscode extension somewhere is messing with this part. All in all, much better! |
|
The duplication issue makes me think if you have two LSPs running in the background… I can't reproduce it in Neovim. Do you have the same issue in Helix? |
|
In helix the inlays look good. The completions seem good. The hovers seem good (exept for markdown formatting). I can't seem to get goto definition working.
|
|
ugh, i take it back. my language server is not set to nu --lsp |
|
ok, i updated my languages.toml to support |
schrieveslaach
force-pushed
the
lsp-document-synchronization
branch
from
590f2f5
to
ef018b7
Compare
|
Strange. I'll try to test the LSP in Helix later. Maybe @jokeyrhyme could test this PR too in Vscode. Meanwhile, I implemented diagnostics:
|
|
That's so cool!!!! |
|
strangely enough, goto-definition on macos with helix is working, as well as hover, but not completions. |
Are you testing on Windows where it is not working? |
|
Tried running it on Linux (nixOS) with helix. Currently no luck. [[language]]
name = "nix"
rulers = [100]
[language.formatter]
command = "/nix/store/iqxdvd5gyckvs6k9al85rgdgqxc6sx0m-nixpkgs-fmt-1.3.0/bin/nixpkgs-fmt"
[[language]]
language-servers = ["nuls"]
name = "nu"
[language-server.nuls]
args = ["--lsp"]
command = "/nix/store/ib059h1skbr272kbn80sig1yk26466za-nu-0.86.1/bin/nu"When I do lsp-restart I get "Language server exited" and in log. |
schrieveslaach
force-pushed
the
lsp-document-synchronization
branch
from
ef018b7
to
d66daa4
Compare
|
I raised a PR against the LSP server crate to make sure that CTRL + C can be handled properly (see rust-lang/rust-analyzer#15894) and updated this PR. |
|
I tried the LSP with Helix on Linux. Seems to work. |
|
I just compiled with the latest changes and it's working in helix for me now too! I also tested goto-definition and hovers. Just to be clear, I have this in my ~/.config/helix/config.toml theme = "dark_plus"
[editor]
mouse = true
true-color = true
#rulers = [80,100,120]
auto-info = true
#auto-pairs = true
gutters = ["diff", "diagnostics", "line-numbers", "spacer"]
[editor.cursor-shape]
insert = "bar"
normal = "block"
select = "underline"
[editor.file-picker]
hidden = false
[editor.lsp]
enable = true #Enables LSP integration. Setting to false will completely disable language servers regardless of language settings.
display-messages = true #Display LSP progress messages below statusline1
auto-signature-help = true #Enable automatic popup of signature help (parameter hints)
display-inlay-hints = true #Display inlay hints2
display-signature-help-docs = true #Display docs under signature help popup
snippets = true #Enables snippet completions. Requires a server restart (:lsp-restart) to take effect after :config-reload/:set.
goto-reference-include-declaration = true #Include declaration in the goto references popup.
[editor.auto-pairs]
'(' = ')'
'{' = '}'
'[' = ']'
'"' = '"'
'`' = '`'
'<' = '>'
Then this in the languages.toml [language-server]
... a bunch of other language servers
nu-lsp = { command = "nu", args = [ "--lsp" ] }
... lots of other stuff
[[language]]
name = "nu"
scope = "source.nu"
injection-regex = "nu"
file-types = ["nu"]
shebangs = ["nu"]
roots = []
comment-token = "#"
indent = { tab-width = 2, unit = " " }
language-servers = [ "nu-lsp" ] |
|
We usually have a pause to landing PRs a few days before a release and a few days after a release. I'd like to land this as soon as we resume landing again. |
|
@fdncred Thanks for the quick iteration on this @schrieveslaach |
|
Let's go! |
|
Thanks for merging it. 😍 |
We're happy to keep chipping away at this thing. Keep up the good work! |
|
The one thing that concerns me about this PR is that we have a git dependency. I'm just hoping the rust-analyzer folks will land that PR soon, because if it doesn't land by our next release (Dec 12th), we will have to revert this PR. |
That was specifically for reacting to Ctrl+C, which I'd suggest probably isn't super critical and could be selectively reverted/removed |
bors
added a commit
to rust-lang/rust-analyzer
that referenced
this pull request
Nov 21, 2023
…ykril Cancelable Initialization This commit provides additional initialization methods to Connection in order to support CTRL + C sigterm handling. In the process of adding LSP to Nushell (see nushell/nushell#10941) this gap has been identified.
bors
added a commit
to rust-lang/rust-analyzer
that referenced
this pull request
Nov 22, 2023
…ykril Cancelable Initialization This commit provides additional initialization methods to Connection in order to support CTRL + C sigterm handling. In the process of adding LSP to Nushell (see nushell/nushell#10941) this gap has been identified.
|
@jokeyrhyme, @fdncred, rust-lang/rust-analyzer#15894 has been merged. Do you want me to update the main branch pointing to the merged version? No release yet on lsp-server. |
We should probably wait for a release. I'm not sure moving from one git dependency to another git dependency is worth it right now. Once there is a crate we can rely on, then we can report to that published crate, but I'm happy they landed the PR so there's hope that they'll make a release. |
|
@schrieveslaach what's the latest on this git dependency? has there been a release that includes it? if so, can we get it in? |
|
@schrieveslaach Thanks. Our next release is schedule for Dec 12. So we may have to land that PR. I appreciate you creating it. |
|
For anyone listening #11252 upgrades to the latest release of lsp-server including the ctrl-c support |
Just landed it. Yay! |
fdncred
pushed a commit
that referenced
this pull request
Dec 8, 2023
<!-- if this PR closes one or more issues, you can automatically link the PR with them by using one of the [*linking keywords*](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword), e.g. - this PR should close #xxxx - fixes #xxxx you can also mention related issues, PRs or discussions! --> # Description Hi! I was playing around and I fixed the formatting in the LSP hover. I _only tested in VS Code using Windows_, if anyone is capable, can you test it on nvim or linux if it works properly? I think markdown shouldn't have any problem The link of the LSP meta issue just for reference #10941 <!-- Thank you for improving Nushell. Please, check our [contributing guide](../CONTRIBUTING.md) and talk to the core team before making major changes. Description of your pull request goes here. **Provide examples and/or screenshots** if your changes affect the user experience. --> # User-Facing Changes Now the LSP hovers markdown properly  <!-- List of all changes that impact the user experience here. This helps us keep track of breaking changes. --> # Tests + Formatting <!-- Don't forget to add tests that cover your changes. Make sure you've run and fixed any issues with these commands: - `cargo fmt --all -- --check` to check standard code formatting (`cargo fmt --all` applies these changes) - `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used` to check that you're using the standard code style - `cargo test --workspace` to check that all tests pass (on Windows make sure to [enable developer mode](https://learn.microsoft.com/en-us/windows/apps/get-started/developer-mode-features-and-debugging)) - `cargo run -- -c "use std testing; testing run-tests --path crates/nu-std"` to run the tests for the standard library > **Note** > from `nushell` you can also use the `toolkit` as follows > ```bash > use toolkit.nu # or use an `env_change` hook to activate it automatically > toolkit check pr > ``` --> # After Submitting <!-- If your PR had any user-facing changes, update [the documentation](https://github.com/nushell/nushell.github.io) after the PR is merged, if necessary. This will help us keep the docs up to date. -->
hardfau1t
pushed a commit
to hardfau1t/nushell
that referenced
this pull request
Dec 14, 2023
hardfau1t
pushed a commit
to hardfau1t/nushell
that referenced
this pull request
Dec 14, 2023
<!-- if this PR closes one or more issues, you can automatically link the PR with them by using one of the [*linking keywords*](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword), e.g. - this PR should close #xxxx - fixes #xxxx you can also mention related issues, PRs or discussions! --> # Description Hi! I was playing around and I fixed the formatting in the LSP hover. I _only tested in VS Code using Windows_, if anyone is capable, can you test it on nvim or linux if it works properly? I think markdown shouldn't have any problem The link of the LSP meta issue just for reference nushell#10941 <!-- Thank you for improving Nushell. Please, check our [contributing guide](../CONTRIBUTING.md) and talk to the core team before making major changes. Description of your pull request goes here. **Provide examples and/or screenshots** if your changes affect the user experience. --> # User-Facing Changes Now the LSP hovers markdown properly  <!-- List of all changes that impact the user experience here. This helps us keep track of breaking changes. --> # Tests + Formatting <!-- Don't forget to add tests that cover your changes. Make sure you've run and fixed any issues with these commands: - `cargo fmt --all -- --check` to check standard code formatting (`cargo fmt --all` applies these changes) - `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used` to check that you're using the standard code style - `cargo test --workspace` to check that all tests pass (on Windows make sure to [enable developer mode](https://learn.microsoft.com/en-us/windows/apps/get-started/developer-mode-features-and-debugging)) - `cargo run -- -c "use std testing; testing run-tests --path crates/nu-std"` to run the tests for the standard library > **Note** > from `nushell` you can also use the `toolkit` as follows > ```bash > use toolkit.nu # or use an `env_change` hook to activate it automatically > toolkit check pr > ``` --> # After Submitting <!-- If your PR had any user-facing changes, update [the documentation](https://github.com/nushell/nushell.github.io) after the PR is merged, if necessary. This will help us keep the docs up to date. -->
dmatos2012
pushed a commit
to dmatos2012/nushell
that referenced
this pull request
Feb 20, 2024
dmatos2012
pushed a commit
to dmatos2012/nushell
that referenced
this pull request
Feb 20, 2024
<!-- if this PR closes one or more issues, you can automatically link the PR with them by using one of the [*linking keywords*](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword), e.g. - this PR should close #xxxx - fixes #xxxx you can also mention related issues, PRs or discussions! --> # Description Hi! I was playing around and I fixed the formatting in the LSP hover. I _only tested in VS Code using Windows_, if anyone is capable, can you test it on nvim or linux if it works properly? I think markdown shouldn't have any problem The link of the LSP meta issue just for reference nushell#10941 <!-- Thank you for improving Nushell. Please, check our [contributing guide](../CONTRIBUTING.md) and talk to the core team before making major changes. Description of your pull request goes here. **Provide examples and/or screenshots** if your changes affect the user experience. --> # User-Facing Changes Now the LSP hovers markdown properly  <!-- List of all changes that impact the user experience here. This helps us keep track of breaking changes. --> # Tests + Formatting <!-- Don't forget to add tests that cover your changes. Make sure you've run and fixed any issues with these commands: - `cargo fmt --all -- --check` to check standard code formatting (`cargo fmt --all` applies these changes) - `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used` to check that you're using the standard code style - `cargo test --workspace` to check that all tests pass (on Windows make sure to [enable developer mode](https://learn.microsoft.com/en-us/windows/apps/get-started/developer-mode-features-and-debugging)) - `cargo run -- -c "use std testing; testing run-tests --path crates/nu-std"` to run the tests for the standard library > **Note** > from `nushell` you can also use the `toolkit` as follows > ```bash > use toolkit.nu # or use an `env_change` hook to activate it automatically > toolkit check pr > ``` --> # After Submitting <!-- If your PR had any user-facing changes, update [the documentation](https://github.com/nushell/nushell.github.io) after the PR is merged, if necessary. This will help us keep the docs up to date. -->
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This commit provides the initial implementation of text document synchronization of the language server protocol: textDocument/didOpen, textDocument/didChange and textDocument/didClose (see #10794)
User-Facing Changes
Completions work better then before (see comment here). The demo show that Nushell opens the line buffer editor, that starts Neovim with Nushell as configured language server which has completions working.
Tests + Formatting
After Submitting