Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Remove lsp4j #13
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
Remove lsp4j #13
Changes from all commits
607a2fe
1dc0dac
7101970
548fd5e
f51c577
229def7
82b34b0
c6cbc76
f94eceb
cbf1610
4d34f0f
4b122fe
b20d0a5
6521c7f
ac6579b
59a9787
9567155
a037776
dcd6e42
6d8929a
e9d18c0
170a651
ce960f9
63a8ce0
be9d336
bd18fcf
1eb7279
b2c3399
01b25d1
61a2dc7
88a8200
b828d68
32fd7cd
c81ca1c
9b46f29
f0f0b58
bef2b0f
0534c09
4f1d4a4
bfe7cc1
7f2be73
c206597
255f1d2
6996fdb
9865b1b
ac2712d
e48f55d
0d97376
38779d3
5d1c339
772f669
786837e
423a8a4
9aefcf6
0826be1
68a5862
baa344f
8b3731a
a3595e1
eed89cd
e58eda1
20c4cbd
597e91e
150115d
48a15ca
f4e2cae
6d02043
f57870f
0d5a9c5
0c5d06b
9f631f2
5dc8183
534d4d6
d93fac0
b1a28ea
117347e
33401b1
d73a556
b36af24
43294db
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing I think we lost using multimethods is that we can't require that clients implement specific methods like we had on protocol, the LSP spec has some methods that all server should provide, they are required methods, like initialize, diagnostics, definition etc. I'm ok for now with this, we can think on how to make required some methods in the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be pedantic, you're not required to implement every method on a protocol, the docs just say that you should. If you don't, you don't get any compile time errors, though you do get a runtime error if you try to call the method you didn't define.
But I understand what you're saying—a protocol is a stronger indication of which methods you're expected to implement.
This is just one of the drawbacks to the multimethods. They also don't give you anywhere to hang extra data about handlers. I can imagine wanting to flag certain messages as cancellable, give them validators to apply to their input or output, or more generally give them middleware style wrappers, etc.
I think the best way to think about this is that one of the typical responsibilities of a server is to route requests. The multimethods work for that, but reitit, Compojure, et al. address the fact that there's more you might want to do with routing.
On the other hand, I haven't run into anything on the clojure-lsp side yet where I really regret the simplicity of the multimethods.
So, I guess my feeling is that multimethods might not be the best design long-term, but I'm not sure I have enough information to commit to another design either.
I've vaguely considered expressing the "router" as a hashmap, keyed by method name, which you pass in to the server when you create it. The server could validate that certain keys were present. That would also give us lots of flexibility about what the values of the hashmap are. They could be functions to call as the handler (much like a multimethod, which maps dispatch values to functions). Or they could be other hashmaps, with a handler function in each. We could add other data over time. If you feel strongly that this is a better solution, I can work on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, the issue is that most clients expect that some methods are always available like diagnostics, but I agree it's best to be a little bit more loose than strict for now.
I think it would work well, but I'm ok trying the multimethods for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also split on this. On the one hand, multimethods bring great simplicity, but our tooling for them is a lot worse. Previously I could find the implementation of the specific message handler with a "find reference", that's gone with multimethods. Additionally, there's no indication of all possible methods I could implement.
I still like the multimethod approach, but maybe this will drive us to add better multimethod stuff into clojure-lsp 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Cyrik we improved a lot defmethods support on clojure-lsp, doesn't find implementations work for that case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing I'm missing from defmethods is a way of seeing references to a specific method. I know that this isn't possible in the general case, which is the whole point of defmethods, but often enough there are hardcoded calls to a specific method that could be shown as a direct reference. Something like
(receive-request "shutdown")
.The other thing is completeness, at least in this case.
But in I also fully agree with @mainej that we should leave this as is for now since these are very minor gripes with the system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Cyrik let me see if I follow...
Suppose we have this multimethod:
At call sites like the following, you know for certain that
(defmethod caps "a" ...)
is being called, not(defmethod caps "b" ...)
.In theory
clojure-lsp
could provide navigation tools to link thea
defmethod to its usage.Of course this isn't possible when it's ambiguous which defmethod is being called:
In these situations
clojure-lsp
couldn't help.Assuming I'm following, I agree, that makes it harder to understand how multimethods, or really any kind of polymorphism, is being used. Polymorphism limits tooling, and that's true of all editors and languages. Maybe the next generation of tooling will offer more insight into polymorphic calls!
For these particular
lsp4clj
multimethods—receive-request
andreceive-notification
—I'd say the problem isn't very big. There's only one place that callsreceive-request
and one that callsreceive-notification
(both inlsp4clj
), so there's not much need to find the call sites.I think there's a stronger argument that the other main entrypoints into
lsp4clj
—send-request
andsend-notification
—exhibit a similar problem, even though they aren't multimethods. They accept the JSON-RPC method as an argument, so it could be difficult to find all the places that, for example, call(lsp.server/send-notification server "$/progress" message)
. Inclojure-lsp
we've worked around this. We have only one place that calls$/progress
, a function calledpublish-progress
. If we want to find all the call sites, we can usefind-references
onpublish-progress
. And in fact, all the calls tosend-request
andsend-notification
are in similar wrapper functions, so it's pretty easy to find references.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mainej Yeah that's the exact case I'm talking about. In my experience, most multimethods are built in this way (using the first param for dispatch). But all of this is very much unrelated so we should probably stop the topic 😄
To your second point: you're correct that this happens more often, I just didn't notice the change yet. Previously it was easy to find references on the
ILSPProducer
methods and you'll now have to do the work yourself. Maybe this could be pulled into lsp4clj as optional entry points. But I know that @ericdallo and I have differing opinions on how much lsp4clj should provide, so I'm also happy to just do that on our end. At some point, I want to publish a library with all the extra utility stuff around lsp4clj anyway (file-management, URL handling, and so on).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could open a new discussion on lsp4clj to see if we could provide something that help you like a different project using lsp4clj under the hood, I think discussing the problem a little bit more would make us understand better the problem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#14
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.