Add arguments to direct logs to various locations #3665
Conversation
| & cmapWithPrio (renderStrict . layoutPretty defaultLayoutOptions . fst) | ||
| -- do not log heap stats to the LSP log as they interfere with the | ||
| -- ability of lsp-test to detect a stuck server in tests and benchmarks | ||
| & if argsTesting then cfilter (not . heapStats . snd . payload) else id |
There was a problem hiding this comment.
We don't need this any more since we're not logging to the client by default.
|
The logs are suddenly so clean and easy to read 🤯 |
michaelpj
force-pushed
the
mpj/log-locations
branch
from
187003c
to
208ccea
Compare
fendor
added
merge me
|
Ah, some tests actually use the logs |
michaelpj
force-pushed
the
mpj/log-locations
branch
from
9553008
to
88f0381
Compare
This adds arguments to HLS to allow the user to select whether to send logs to any or all of: - a file - stderr - the client Importantly, we can toggle off the default stderr logging, so the vscode extension can turn it off to avoid the double logging that arises from logging to both the client and stderr. I've set the default to _not_ log to the client. This is a change of behaviour (today we log to the client by default), but I think it gives the best experience by default, since most clients do show stderr output somewhere, and then we probably want to make a case-by-case decision on whether to use the client logging instead.
michaelpj
force-pushed
the
mpj/log-locations
branch
from
88f0381
to
fc13e05
Compare
| @@ -138,12 +140,40 @@ arguments plugins = GhcideArguments | |||
| <> help "Sets the log level to Debug, alias for '--log-level Debug'" | |||
| ) | |||
| ) | |||
| <*> optional (strOption | |||
| (long "logfile" | |||
| <> short 'l' | |||
There was a problem hiding this comment.
I would suggest keeping the short form for at least one release cycle.
I just got bitten by this, and had to re-configure my emacs to use it
:custom
(lsp-haskell-server-args `("-d" "--log-file" ,lsp-haskell-server-log-file))There was a problem hiding this comment.
Ah sorry, that was an oversight, I meant to leave the old options in! I left the old spelling but missed the short option.
There was a problem hiding this comment.
Although I don't think leaving it in for a release cycle helps all that much: it just means that people will be surprised by it one cycle later. It's only useful if you're switching between and old and new version.
There was a problem hiding this comment.
(... then why am I leaving the old spelling in? I guess we could just never remove it)
There was a problem hiding this comment.
The one release cycle thing is so you could update clients to use the new one, but if people still ran an older server it would still work. Then in time get rid of it.
To me, keeping the short form is the best option
This adds arguments to HLS to allow the user to select whether to send logs to any or all of:
Importantly, we can toggle off the default stderr logging, so the vscode extension can turn it off to avoid the double logging that arises from logging to both the client and stderr.
I've set the default to not log to the client. This is a change of behaviour (today we log to the client by default), but I think it gives the best experience by default, since most clients do show stderr output somewhere, and then we probably want to make a case-by-case decision on whether to use the client logging instead.