Refactor LSP logger and log via window/logMessage also #2758
Conversation
michaelpj
force-pushed
the
mpj/lsp-logging
branch
from
5b61944
to
7657312
Compare
ghcide/src/Development/IDE/Main.hs
Outdated
| -> Maybe (MVar (LSP.LanguageContextEnv Config)) | ||
| -- ^ Variable to be filled with the LSP environment, useful for tools that need this outside | ||
| -- the scope of runLanguageServer | ||
| -> Arguments | ||
| -> IO () | ||
| defaultMain recorder lspEnvVar Arguments{..} = withHeapStats (cmapWithPrio LogHeapStats recorder) fun |
There was a problem hiding this comment.
The MVar is an implementation detail, better hide it behind a callback, or a hook to run an Lsp action on initialisation.
And please move it to the Arguments record, to keep the API simple and minimise the breakage
There was a problem hiding this comment.
I wasn't sure about whether to put it in Arguments, in some places there seems to be an assumption that it reflects CLI arguments. But yeah, it seems better in there.
There was a problem hiding this comment.
I ended up putting it in the Arguments record for ghcide but not HLS: the HLS Arguments are very tightly coupled to argument parsing, expect to be Show-able etc, so it really didn't fit.
There was a problem hiding this comment.
I meant the Arguments datatype passed to defaultMain, that is Development.IDE.Main.Arguments.
Sorry I wasn't more specific.
|
Please keep in mind what happens with the command line interface, where there is no LSP environment. |
That should be fine, although it does suggest to me that we should use a bounded queue for the backlog: otherwise it will just accumulate logs forever because it can never send them. |
michaelpj
force-pushed
the
mpj/lsp-logging
branch
from
88cf0ae
to
16dd74e
Compare
michaelpj
changed the title
|
Okay, this is now a much more modest refactoring that still uses the plugin technique to install the handler, and adds logging via Also I officially give @pepeiborra 1 I-told-you-so point ;) |
* Refactor LSP logger and log via `window/logMessage` * Skip logging notifications in tests Co-authored-by: Pepe Iborra <pepeiborra@gmail.com>
Also adds a logger that forwards on messages via
window/logMessage.