João Távora <
address@hidden> schrieb am Fr., 18. Mai 2018 um 18:28 Uhr:
Eli Zaretskii <address@hidden> writes:
>> From: João Távora <address@hidden>
>> Date: Thu, 10 May 2018 23:34:40 +0100
>>
>> I'd like to add Eglot, my new package, to GNU ELPA.
>>
>> Eglot, for Emacs Polyglot, is an Emacs client for the LSP, or Language
>> Server Protocol (https://microsoft.github.io/language-server-protocol/).
>
> FWIW, I think LSP support should be part of the core Emacs
> distribution, as it's an infrastructure necessary for modern support
> of programming modes. (I didn't yet look at the package, so if it has
> separate infrastructure and application levels, perhaps only the
> infrastructure layers should be in Emacs.)
Hi again, Eli
So, off-list Stefan likewise suggested that some level of LSP
"infrastructure" is integrated into Emacs, which could lead to merging
eglot.el and other LSP clients like lsp-mode.el in the future.
As JSONRPC support is a mandatory component of LSP, I've extracted a
library implementing the JSONRPC spec (see www.jsonrpc.org) out of
eglot.el.
I propose this library, jrpc.el, for Emacs core (or GNU ELPA, or
both). I hope people can review it, comment on it, and propose changes,
particularly developers of other existing LSP-modes. I attach to the
end of this mail with a reasonably complete ";;; Commentary" header,
including a simple usage example.
Thanks. I think that separating out JSONRPC from LSP and having a JSONRPC library in core are highly valuable additions that I'd strongly support. Some review comments follow; sorry that there are so many, but I think as a foundational library jrpc.el should be as high-quality as possible:
- Please add extensive unit and integration tests. There are lots of corner cases (what happens when the server times out, sends an invalid or incomplete response, sends responses in the wrong order, etc.) that should all be tested.
- Please remove all defcustoms and defvars. jrpc.el is a low-level library, it doesn't make sense for it to maintain global state or be globally customizable. Rather, the timeout could either be a mandatory argument, or a reasonable constant default could be selected, and the other variables should be per-process.
- Also please remove `jrpc-current-process'. There is no general notion of a "current" process; clients should maintain process state themselves and pass process objects explictly. It seems this is only used as completion helper anyway, so I think you can remove it entirely and replace it with something based on `process-list'.
- Please don't use the `error' symbol directly. Instead, define your own error symbols for each error category using `define-error'. Only then clients have the chance to catch and handle errors.
- `jrpc-define-process-var' isn't JSONRPC-specific, it should be moved somewhere else (subr.el)?
- Instead of defining process variables, consider using a structure or EIEIO class to encapsulate all needed state for the process client.
- Please make it possible to run servers remotely. Unfortunately this isn't possible with `make-process', so you have to use `start-file-process' instead if `default-directory' is remote. As an alternative to using `default-directory', consider passing a REMOTE-DIRECTORY argument to `jrpc-connect' so that the behavior doesn't depend on ambient context.
- I think many of the names can be internal (i.e. contain a double dash) unless they are explicitly part of the public API.
- Probably jrpc-async-request should just return nil. Or is there a situation where callers have to know about IDs or timers? I think those should be treated as implementation details and not be exposed to clients.
- Please use hashtables or alists instead of plists. There are many more functions to operate on hashtables and alists, and json.c doesn't support plists.
- The HTTP-style enveloping (using HTTP headers to separate responses) is technically not part of JSONRPC. You could either add support for other enveloping methods (though I'm not aware of any), or at least call this out in the documentation.
- I think `string-bytes' isn't guaranteed to return the number of UTF-8 bytes. Rather you should encode the string explicitly to a unibyte string using `encode-coding-string', and then use `no-conversion' for the process coding systems.
- Consider also sending (and parsing) a Content-Type header: Content-Type: application/vscode-jsonrpc; charset=utf-8.
- I don't quite understand how `jrpc-ready-predicates' is supposed to be used. Are clients supposed to have some form of server-specific sidechannel to detect when the server is ready? It might be simpler to solve this problem in Emacs core, e.g. by adding a timeout to `process-send-string'.
- I'd call the library jsonrpc.el (or json-rpc.el or jsonrpc2.el or...) if that name isn't already taken.
Thanks again for working on this!