add support for VS Code's LogOutputChannel API #1624
Conversation
|
@microsoft-github-policy-service agree |
| @@ -348,9 +348,9 @@ InlineCompletionMiddleware & TextDocumentContentMiddleware & GeneralMiddleware; | |||
| export type LanguageClientOptions = { | |||
| documentSelector?: DocumentSelector | string[]; | |||
| diagnosticCollectionName?: string; | |||
| outputChannel?: OutputChannel; | |||
| outputChannel?: LogOutputChannel | OutputChannel; | |||
There was a problem hiding this comment.
Writing LogOutputChannel | OutputChannel instead of OutputChannel isn't really necessary but I did it so that the log output channel is more prominently mentioned in the source code. I can undo this if undesired.
|
@Techatrix thanks for the PR. Just wanted to let you know that I am OOF at that it has to wait until I am back. |
| if (!this._outputChannel) { | ||
| this._outputChannel = Window.createOutputChannel(this._clientOptions.outputChannelName ? this._clientOptions.outputChannelName : this._name); | ||
| this._outputChannel = Window.createOutputChannel(this._clientOptions.outputChannelName ? this._clientOptions.outputChannelName : this._name, { log: true }); |
There was a problem hiding this comment.
There was some discussion in #1116 that this was not ideal for existing languages that are already implementing their own filtering by severity in another way -- this is creating a new UI that they won't be listening to and might be confusing. I'd suggest that we either:
- Don't add log: true here, and expect languages that have opted into the new logging support to pass their channel accordingly. (For the C# extension, this is what we're already doing, so this works great for us.)
- Add the boolean to clientOptions, so languages opt-in that way.
There was a problem hiding this comment.
I guess this depends on whether a breaking change is wanted here or not. Changing the default here can have the advantage that it encourages extensions to use the new logging utilities that more closely align with VS Code and the LSP Spec. If a breaking change undesired, I think that the first option is reasonable.
There was a problem hiding this comment.
There was one request to not make the breaking change in the bug, and if an extension wants to opt-in it's not very hard to do so. If anything else, it might be worth merging this in without the break and then the break can be discussed separately.
(I'll defer to @dbaeumer if he just wants to take the break or not.)
| function isLogOutputChannel(channel: OutputChannel): channel is LogOutputChannel { | ||
| const candidate = channel as LogOutputChannel; | ||
| return candidate.trace !== undefined && candidate.debug !== undefined && candidate.info !== undefined && candidate.warn !== undefined && candidate.error !== undefined; | ||
| } |
There was a problem hiding this comment.
Consider putting this in some common place.
There was a problem hiding this comment.
Any suggestions on where a good place for this would be? I'm not very familiar with the codebase and couldn't find a location that seems reasonable to put this.
Techatrix
force-pushed
the
client-log-output-channel
branch
from
6cec2e8 to
fdb18fc
Compare
Techatrix
force-pushed
the
client-log-output-channel
branch
from
fdb18fc to
5c218a5
Compare
fixes #1116
This will improve the language client's support for using LogOutputChannel instead of OutputChannel.
I tested these changes with the ZLS language server which supports logging with window/logMessage or optionally through stderr. To avoid making the PR description too long, I included only images of the final output. If text output or reproducible steps are wanted, I’d be happy to add them as well.
window/logMessageBefore, OutputChannel
After, OutputChannel
Before, LogOutputChannel
After, LogOutputChannel
stderr logging
Before, OutputChannel
The
info ( main )part is added by the server when logging to stderr.After, OutputChannel
Before, LogOutputChannel
After, LogOutputChannel
This has since been changed to use
[INFO]instead of[ERROR].I personally don’t have a strong preference between
LogOutputChannelandOutputChannel, which is why I chose to retain support forOutputChannelso that extension authors to decide which one better suits the output of their language server. However, if maintaining support forOutputChannelis not desirable, I’m open to removing it.In the following example, I modified the server to intentionally crash so that it writes a stack trace to stderr. This is meant to highlight that mixing
window/logMessageand stderr is possible and produces readable output.After, LogOutputChannel,
window/logMessage+ stderr