Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement console log window messages, update lsp-ws-connection #606

Merged
merged 7 commits into from
May 30, 2021

Conversation

krassowski
Copy link
Member

@krassowski krassowski commented May 23, 2021

References

Fixes #601 and updates lsp-ws-connection:

Code changes

  • use import type if possible
  • update:
    • "vscode-jsonrpc": "^6.0.0",
    • "vscode-languageserver-protocol": "^3.16.0",
    • "vscode-languageserver-types": "^3.16.0",
    • "vscode-ws-jsonrpc": "0.2.0"
  • remove unused code from lsp-ws-connection
  • deprecate the emit() and getter based API for lsp-ws-connection
  • add new API with well-typed:
  public serverNotifications: ServerNotifications;
  public clientNotifications: ClientNotifications;
  public clientRequests: ClientRequests;
  public serverRequests: ServerRequests;

User-facing changes

Screenshot from 2021-05-23 17-43-24

Backwards-incompatible changes

Chores

  • linted
  • tested
  • documented
  • changelog entry

Comment on lines 50 to 61
this.notifications = {
$: {
logTrace: new Signal(this),
setTrace: params => {
this.connection.sendNotification('$/setTrace', params);
}
},
window: {
showMessage: new Signal(this),
logMessage: new Signal(this)
}
};
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CC @bollwyvl . I know this is a different pattern from the existing one, but trying to experiment on doing it better. I also know the dream would be to generate typings from a JSON file; the handlers for signals could be also generated from JSON, but not sure if those would work for all handlers + some signals might be performance-critical and maybe using direct callbacks rather than lumino Signal could be better.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Welp, even lower in the stack is the lumino Message bus. Nobody talks about it much, but it is extremely robust, and what makes all the core widget stuff very smooth.

But performance-critical is as performance-critical-is-measured... we'd need some no-fooling benchmarks before stepping too far off something that at least quacks likes other parts of the stack.

Co-authored-by: bollwyvl
Co-authored-by: krassowski
Comment on lines 24 to 128
/** Server notifications */
export enum ServerNotification {
PUBLISH_DIAGNOSTICS = 'textDocument/publishDiagnostics',
SHOW_MESSAGE = 'window/showMessage',
LOG_TRACE = '$/logTrace',
LOG_MESSAGE = 'window/logMessage'
}

/** Client notifications */
export enum ClientNotification {
DID_CHANGE = 'textDocument/didChange',
DID_CHANGE_CONFIGURATION = 'workspace/didChangeConfiguration',
DID_OPEN = 'textDocument/didOpen',
DID_SAVE = 'textDocument/didSave',
INITIALIZED = 'initialized',
SET_TRACE = '$/setTrace'
}

/** Server requests */
export enum ServerRequests {
REGISTER_CAPABILITY = 'client/registerCapability',
SHOW_MESSAGE_REQUEST = 'window/showMessageRequest',
UNREGISTER_CAPABILITY = 'client/unregisterCapability'
}

/** Client requests */
export enum ClientRequest {
COMPLETION = 'textDocument/completion',
COMPLETION_ITEM_RESOLVE = 'completionItem/resolve',
DEFINITION = 'textDocument/definition',
DOCUMENT_HIGHLIGHT = 'textDocument/documentHighlight',
DOCUMENT_SYMBOL = 'textDocument/documentSymbol',
HOVER = 'textDocument/hover',
IMPLEMENTATION = 'textDocument/implementation',
INITIALIZE = 'initialize',
REFERENCES = 'textDocument/references',
RENAME = 'textDocument/rename',
SIGNATURE_HELP = 'textDocument/signatureHelp',
TYPE_DEFINITION = 'textDocument/typeDefinition'
}
}

export interface IServerNotifyParams {
[Method.ServerNotification.LOG_MESSAGE]: lsp.LogMessageParams;
[Method.ServerNotification.LOG_TRACE]: rpc.LogTraceParams;
[Method.ServerNotification.PUBLISH_DIAGNOSTICS]: lsp.PublishDiagnosticsParams;
[Method.ServerNotification.SHOW_MESSAGE]: lsp.ShowMessageParams;
}

export interface IClientNotifyParams {
[Method.ClientNotification
.DID_CHANGE_CONFIGURATION]: lsp.DidChangeConfigurationParams;
[Method.ClientNotification.DID_CHANGE]: lsp.DidChangeTextDocumentParams;
[Method.ClientNotification.DID_OPEN]: lsp.DidOpenTextDocumentParams;
[Method.ClientNotification.DID_SAVE]: lsp.DidSaveTextDocumentParams;
[Method.ClientNotification.INITIALIZED]: lsp.InitializedParams;
[Method.ClientNotification.SET_TRACE]: rpc.SetTraceParams;
}

export interface IServerRequestParams {
[Method.ServerRequests.REGISTER_CAPABILITY]: lsp.RegistrationParams;
[Method.ServerRequests.SHOW_MESSAGE_REQUEST]: lsp.ShowMessageRequestParams;
[Method.ServerRequests.UNREGISTER_CAPABILITY]: lsp.UnregistrationParams;
}

export interface IServerResult {
[Method.ServerRequests.REGISTER_CAPABILITY]: void;
[Method.ServerRequests.SHOW_MESSAGE_REQUEST]: lsp.MessageActionItem | null;
[Method.ServerRequests.UNREGISTER_CAPABILITY]: void;
}

export interface IClientRequestParams {
[Method.ClientRequest.COMPLETION_ITEM_RESOLVE]: lsp.CompletionItem;
[Method.ClientRequest.COMPLETION]: lsp.CompletionParams;
[Method.ClientRequest.DEFINITION]: lsp.TextDocumentPositionParams;
[Method.ClientRequest.DOCUMENT_HIGHLIGHT]: lsp.TextDocumentPositionParams;
[Method.ClientRequest.DOCUMENT_SYMBOL]: lsp.DocumentSymbolParams;
[Method.ClientRequest.HOVER]: lsp.TextDocumentPositionParams;
[Method.ClientRequest.IMPLEMENTATION]: lsp.TextDocumentPositionParams;
[Method.ClientRequest.INITIALIZE]: lsp.InitializeParams;
[Method.ClientRequest.REFERENCES]: lsp.ReferenceParams;
[Method.ClientRequest.RENAME]: lsp.RenameParams;
[Method.ClientRequest.SIGNATURE_HELP]: lsp.TextDocumentPositionParams;
[Method.ClientRequest.TYPE_DEFINITION]: lsp.TextDocumentPositionParams;
}

export interface IClientResult {
[Method.ClientRequest.COMPLETION_ITEM_RESOLVE]: lsp.CompletionItem;
[Method.ClientRequest.COMPLETION]: AnyCompletion;
[Method.ClientRequest.DEFINITION]: AnyLocation;
[Method.ClientRequest.DOCUMENT_HIGHLIGHT]: lsp.DocumentHighlight[];
[Method.ClientRequest.DOCUMENT_SYMBOL]: lsp.DocumentSymbol[];
[Method.ClientRequest.HOVER]: lsp.Hover;
[Method.ClientRequest.IMPLEMENTATION]: AnyLocation;
[Method.ClientRequest.INITIALIZE]: lsp.InitializeResult;
[Method.ClientRequest.REFERENCES]: Location[];
[Method.ClientRequest.RENAME]: lsp.WorkspaceEdit;
[Method.ClientRequest.SIGNATURE_HELP]: lsp.SignatureHelp;
[Method.ClientRequest.TYPE_DEFINITION]: AnyLocation;
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cherry-picked from #278 but Methods now grouped into enums for run-time generation of signals and handlers, CC @bollwyvl

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! Someday we'll be able to close that one out! Over in jupyter_server meeting, we talked about some approaches for moving forward for improving reuse of specs as docs (yay) and simple type packages (no ORMs, future-imperiled pydantic stuff, etc).

I don't know how this would work out here, as these are sometimes a good deal more complicated.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is to have strong separation between server/client, notification/request as otherwise those get mixed up. Also, this implementation does not require manual typing anymore. Eventually I would like to define an interface of ILSPCompletion (as you did in #278) and make everything else an implementation detail. I hope that this would allow to smoothly add the kernel approach from #278 too (as an alternative implementation of the interface).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, well... the upstream LSP spec hides that particular piece of information (whether a message is in a request or a notification) behind some emoji. In the various attempts at building schema for it, I had to parse that business and build actual models of the exchange sequences: not so much fun.

In the jupyter_server discussion, the idea was to get to where we can incrementally define robust, declarative, language-agnostic specs (JSON Schema, OpenAPI, AsyncAPI, kaitaistruct) for different parts of the architecture, from which many language "bindings" could be built, as close to the standard library of that language as possible. Unfortunately, the place I called out initially was the ContentsManager API... which would drag along nbformat, and a subset of kernelspec. From there, the kernel session REST, and eventually the kernel interchange itself, could be documented... that would either be the baseline for the LSP Comm approach, or at least give us enough ground to stand on to document our "special" websocket.

Semi-related, if kernels were better able to describe, and report, their special syntaxes, and ideally a syntax highlighting approach... well, that sure would make a composite language document more understandable, and a lot easier than ginning up custom typescript extensions.

More concretely here... I think really supporting multiple sources (of any of these things) will end up needing a more explicit document/mark model, a la #500... in the case of a completion, documentation requests, etc. these would be extremely short-lived, but one should see whatever is available as fast as possible, but know that some other sources might be coming, and be able to configure the hell out of them, e.g.

In []: df.<TAB>
       |----------------------------------------|
       | apply                            [lsp] |
       | some_col                         [jkm] |
       |----------------------------------------|
       | Sources [lsp] 300ms  [jkm] 100ms [gear]| 

@krassowski krassowski merged commit 22bf7b3 into master May 30, 2021
@krassowski krassowski deleted the add-log-window branch May 30, 2021 20:30
@krassowski krassowski added this to the 3.7 milestone Aug 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support window/logMessage
2 participants