Fixes line encoding for strict clients (neovim) #24
Conversation
|
Ope, I think I forget to fix the tests |
|
There's a few rather odd changes that come from my use of nvim-paredit with strict paren matching enabled. It's mostly pedantic cleanup of tabs and removing some odd vestigial bits of code. If undesired, simply remove those changes. |
test/test-main.janet
Outdated
There was a problem hiding this comment.
This breaks the tests in this file.
project.janet
Outdated
| @@ -5,7 +5,8 @@ | |||
| :dependencies ["https://github.com/janet-lang/spork.git" | |||
| "https://github.com/CFiggers/jayson.git" | |||
| "https://github.com/ianthehenry/judge.git" | |||
| "https://github.com/CFiggers/cmd.git"]) | |||
| "https://github.com/CFiggers/cmd.git" | |||
| "https://github.com/CFiggers/jgraph.git"]) | |||
There was a problem hiding this comment.
jgraph isn't really a dependency of this project, it's just referenced in some sample code used to test the formatter. I wouldn't include it here (if having it as part of the formatting example is causing problems, I'd rather get a new formatting sample).
There was a problem hiding this comment.
FWIW, I had noticed that without jgraph installed the tests (at least those invokable via jpm test) don't run without errors:
$ jpm test
running test/test-format-file-after.janet ...
error: could not find module jgraph:
build/jgraph.so
/home/user/.local/lib/janet/jgraph.jimage
/home/user/.local/lib/janet/jgraph.janet
/home/user/.local/lib/janet/jgraph/init.janet
/home/user/.local/lib/janet/jgraph.so
in require-1 [boot.janet] on line 3028, column 20
in import* [boot.janet] on line 3072, column 15
in thunk [test/test-format-file-after.janet] (tail call) on line 3, column 1
non-zero exit code in test/test-format-file-after.janet: 1
running test/test-format-file-before.janet ...
error: could not find module jgraph:
build/jgraph.so
/home/user/.local/lib/janet/jgraph.jimage
/home/user/.local/lib/janet/jgraph.janet
/home/user/.local/lib/janet/jgraph/init.janet
/home/user/.local/lib/janet/jgraph.so
in require-1 [boot.janet] on line 3028, column 20
in import* [boot.janet] on line 3072, column 15
in thunk [test/test-format-file-before.janet] (tail call) on line 3, column 1
non-zero exit code in test/test-format-file-before.janet: 1
running test/test-integration.janet ...
running test/test-lookup.janet ...
running test/test-main.janet ...
Starting LSP
May be that's what you meant by:
it's just referenced in some sample code used to test the formatter.
There was a problem hiding this comment.
Yeah, I don't use jpm test in this project—I'm using ianthehenry/judge for everything (admittedly not all of those are passing at the moment either... 😅)
There was a problem hiding this comment.
The tests were running with jpm test when I ran them (with jgraph installed). I didn't realize judge had its own executable.
There was a problem hiding this comment.
Tests set up with Judge ((test) and (deftest) macros) don't mean anything when run with jpm test—jpm test essentially just evaluates .janet files, it doesn't do any special test running. The (test) and (deftest) macros in Judge evaluate to no-ops when you're not using the judge binscript. So even failing Judge tests will not complain when jpm test is run.
There was a problem hiding this comment.
FWIW, I disabled jpm test in a project like this.
One could customize output to indicate the use of other options I suppose :)
There was a problem hiding this comment.
Could we use jpm to invoke judge instead? I'm usually a pretty big fan of consistent interfaces :)
There was a problem hiding this comment.
judge's documentation mentions a specific way it can be used via project.janet:
You can also add this to your project.janet file:
(task "test" [] (shell "jpm_tree/bin/judge"))
To run Judge with a normal jpm test invocation.
May be that or something similar is suitable for janet-lsp's case?
There was a problem hiding this comment.
judge's documentation mentions a specific way it can be used viaproject.janet:You can also add this to your project.janet file:
(task "test" [] (shell "jpm_tree/bin/judge"))
To run Judge with a normal jpm test invocation.
May be that or something similar is suitable for janet-lsp's case?
I'm trying to get this working and right now I'm only getting obscure build errors whenever this task is included in project.janet.
|
Thanks for the PR. I'm glad you're enjoying the LSP! Could you provide more detail on what "ignore responses and behave very strangely" looks like in practice? And how you're confident that this change fixes the problem? Before merging this I'll need to test it with Janet++ to make sure it doesn't break anything in the VS Code implementation. |
I believe it was effectively this:
from the LSP Spec The neovim LSP client would start the LSP process and send an Neovim would show that the server was running (confirmed via
So without any new requests from the client, the LSP server isn't able to... well do anything, and so none of the LSP functionality works. With my changes (really just the line ending changes, the rest are unrelated), the expected behavior happens and I'm able to get docstrings and stuff (notice, the buffer ID):
I believe neovim is somewhat picky here and always expects |
|
I'll fix up the PR after work today. I'm not able to test on VSCode at the moment, but I can also attempt that if someone else doesn't get to it first. |
|
Hmmm... I've been using Janet LSP together with Neovim for some time now (at least a couple months) and haven't encountered that issue. (Just to state the obvious, I'm still interested in fixing this because if you're hitting it then other folks might be as well. 🙂) I'm curious, are you running under Windows or a Unix-like (Mac, Linux, BSD)? |
I'm using Linux (NixOS, specifically). I have a mac that I can test on later. |
|
It may also be a bug or change in the latest release of nvim, I'm using 0.10 |
|
Interestingly, I do not appear to have this problem on MacOS. Perhaps this is a platform-specific issue! I'll see if other Linux distros give me trouble |
|
On a mostly unrelated note, nvim-lspconfig now has a basic config for janet_lsp |
I was unable to reproduce on a Ubuntu VM so it has to be something funky with NixOS. Perhaps it's a locale thing? I'm really not sure. |
|
I'm starting to think this is somehow a locale or kernel-level issue. I'm running
Will test with some other platforms. It could be that this is new behavior in neovim itself. |
|
Ok, I have reproduced it using the Regardless, the |
|
Some more analysis: the neovim RPC client always splits on |
That's pretty definitive. 😁 Thanks for finding that. Once we get the other stuff cleaned up I'll be happy to merge. |
The line ending is *always* \r\n as far as neovim is concerned. Futher, these values are effectively constants, no reason to wrap them in function calls.
|
Alright, should be cleaned up now. Let me know if there's anything else this needs! |
|
It looks like you still have a call to |
|
Oh huh, whoops |
|
As it turns out, it seems like I'm going to need to revert this pull request—whatever the specification may say, experimentally Windows needs I don't know exactly why this would be— at this point I suspect it has to do with the way STDIO works on Windows? One debug test I did had four separate line breaks showing up, making me think that perhaps Windows is somehow interpreting both The current master branch, which is using Notably, it looks like I myself introduced a bug related to this back in 90a33d2 , which might be why you first encountered issues with carriage returns. At any rate, I'll be pushing a patch to master later today. |
|
I suspect there is something else in the chain turning |
|
Yep, that looks very similar indeed. Great find! |
The line ending is always \r\n as far as neovim is concerned. Futher, these values are effectively constants, no reason to wrap them in function calls.
Without this change, neovim will ignore responses and behave very strangely.
I also intend to submit a PR to nvim-lspconfig to do some basic setup.