Rewrite msg dispatch #701
Rewrite msg dispatch #701
Conversation
|
I've had a quick look - this is pretty much how I'd pictured it and seems much more sane. My only query for now is why |
My hope is that I can more easily add a mock testing framework that way, so that wen can write unit tests for each message handler without ever needing to deal with real communication in those tests. If that turns out to not work we can still change things back so that these functions use the instance that is stored in the server. |
|
@ZacLN I'm stuck with something around the construction of instances from JSON in this branch. In particular, if you run tests, things don't work because there is some deserialize problem where it tries to convert a |
|
Yep I'll take a look. The json converter code is pretty delicate... |
|
Ah, OK. You're trying to convert |
|
Ah, yes, that is it! Thanks :) |
| @@ -239,20 +259,45 @@ function Base.run(server::LanguageServerInstance) | |||
| Base.display_error(stderr, err, bt) | |||
| end | |||
| end | |||
|
|
|||
| msg_dispatcher = JSONRPC.MsgDispatcher() | |||
There was a problem hiding this comment.
Should the following maybe sit in msgdefs.jl?
There was a problem hiding this comment.
I don't think so, the idea is that this is specific to this particular server who handles things, whereas the msgdefs.jl is really the file that defines the protocol.
| request = parse(JSONRPC.Request, msg) | ||
|
|
||
| res = process(request, server) | ||
| msg = message.msg |
There was a problem hiding this comment.
Not introduced by this PR but I feel like message.msg indicates that one of these is incorrectly named
There was a problem hiding this comment.
Yeah, but maybe better solved in a follow up PR.
| @@ -145,12 +143,12 @@ function expand_inline_func(x, id, server) | |||
| end | |||
| newtext = string(newtext, "\nend\n") | |||
| tde = TextDocumentEdit(VersionedTextDocumentIdentifier(file._uri, file._version), TextEdit[TextEdit(Range(file, offset .+ (0:func.fullspan)), newtext)]) | |||
| JSONRPCEndpoints.send_request(server.jr_endpoint, "workspace/applyEdit", ApplyWorkspaceEditParams(missing, WorkspaceEdit(nothing, TextDocumentEdit[tde]))) | |||
| JSONRPC.send(conn, workspace_applyEdit_request_type, ApplyWorkspaceEditParams(missing, WorkspaceEdit(nothing, TextDocumentEdit[tde]))) | |||
There was a problem hiding this comment.
Unrelated but I think the first arg to WorkspaceEdit should be missing rather than nothing. Same thing a few lines above
There was a problem hiding this comment.
Best solved in a new PR, I'd say?
|
Thanks! I either fixed or commented on the individual points. Some of these I agree with, but it probably would be easier to do those in follow up PRs. |
This is a rewrite how messages are dispatched to handler functions. I mostly modeled this after the vscode-jsonprc package.
One main goal was to move all of the JSON aspects out of the julia functions that actually handle a request or notification. I think that will make it much easier to write tests for those functions.
@ZacLN not ready for a detailed review, but if you want to have a quick look and give feedback on the general direction, that might be a good moment.
Requires julia-vscode/JSONRPC.jl#4.
TODO
Remove endpoint fromEDIT we can do this later.lint!function