-
Notifications
You must be signed in to change notification settings - Fork 118
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
Add a new custom command ocamllsp/hoverExtended with support for variable verbosity #561
Conversation
Wow, this works like a charm! Thanks! Regarding implementation:
Screen.Recording.2021-11-28.at.19.45.13.mov |
There is a somewhat related PR in merlin repo btw: ocaml/merlin#1374 |
I understand the appeal of doing it on the server - we do it once and it automatically works everywhere. Unfortunately, there's also a serious downside. The LSP server is a basic block for editors to build functionality on top of, and this behavior is making a pretty basic request unpredictable. It might not matter for users of vscode, but in other editors, lsp requests can be scripted to provide higher level functionality and this is a wrench thrown in that work. So I suggest we leave basic requests alone and do one of:
|
Ok I can do that. Any suggestion on the name? HoverVerbose? HoverSmart? Thanks to both of you for the quick response |
Regarding designing a custom hover request, I think this may be an opportunity to design the request to also support selections (ie show types for selections) that was also discussed in the ocaml discuss thread mentioned earlier I think. Reference of
|
e58a8a6
to
1464507
Compare
1464507
to
c2bec75
Compare
I've started to work on adding a new command. I'm not sure exactly how much work it would require in the client to become usable. I am scared that it becomes very troublesome to use. But in the process I found that it is possible to have multiple hover providers, which could be an alternative way to have this feature. One provider could give the normal type, one could give the doc, one could give a more and more verbose answer. But I don't know if one server can have multiple hover providers. Anyway, in the process of adding a new command I had to duplicate quite a lot of code from |
I haven't looked into this too closely, but I was under assumption that for vscode client, we simply add our own hover provider that uses the custom hover request-response and turn off the provider that uses the default LSP request for hovers. Does this make sense?
This PR should help with this. |
38fa2c0
to
259bf6e
Compare
A few months later, I'm back with an updated version of this PR. There's no state on the server anymore, it is left to the client. I'm not handling selections (solving one problem at a time). And I haven't tried the change in an editor yet. Sadly it looks like my change to send the verbosity to merlin no longer works ocaml-lsp/ocaml-lsp-server/src/document.ml Lines 281 to 291 in 259bf6e
It was fine it my original commit Khady@a25a778
|
actually if I hardcode a verbosity of 100 it seems to do something (it breaks other tests). So maybe it is my test that isn't correctly written? |
259bf6e
to
d8e0212
Compare
Since we're creating a new request there should be no harm in keeping the state on the server side. I would hate for every client to have to implement the same thing. AFAIU, Rudi is not against it as long as we have a custom request for it:
|
At this point I don't have a strong opinion anymore. As soon as it's a new request it requires some extra work in every client anyway. I guess the new request can even support both. If verbosity is passed as an argument then it is forwarded to merlin. If it is omitted some server side state is used instead. Pleases everyone? |
Sounds fine to me.
It's a new and custom request, and you're the one taking the initiative, so feel free to make the final call. |
* master: fix(go-to-def): report error in response (ocaml#899) Update readme (ocaml#895) chore(nix): update flakes and dune-release (ocaml#894) chore: remove ocamlformat-rpc (ocaml#892) chore(nix): pass opam-repository and nixpkgs (ocaml#893) chore: unvendor ocamlc-loc (ocaml#869) fix: merlin document safety (ocaml#890) chore: more precise CHANGES (ocaml#889) fix: diagnostics on non dune files (ocaml#887) refactor: remove Document.is_merlin (ocaml#888) fix: symbols in non merlin docs (ocaml#886) refactor: style tweaks in document_symbol (ocaml#885) fix: handle incorrect document types (ocaml#884) Ignore unknown config tags (ocaml#883) Make sure nodejs packages required are installed chore: prepare 1.14.0 Don't let opam ask when not needed Allow copy-and-paste of contributing instructions Add code action for inlining (ocaml#847) Add note about protocol extensions to the readme
74890ff
to
74d42f7
Compare
I think I addressed all your comments. For the ocaml e2e tests, I've been able to write something and get it running. But I'm a bit lost. It works fine for the default hover request, even when in extended mode thanks to the environment variable. But for hoverExtended I always receive a null answer. Am I missing some kind of special wait that must be done for custom requests only? |
|
Ok I figured out the problem.There was a hardcoded position that I forgot to remove... |
* master: chore(nix): update flakes (ocaml#915) chore(merlin): subrepo to submodule (ocaml#914) refactor: add signature_help mli (ocaml#913) fix: correctly use merlin's pipeline (ocaml#904) feature(lsp): add workspace/diagnostic/refresh (ocaml#910) refactor: add mli's to test helpers (ocaml#912) fix: client capabilities and diagnostics (ocaml#908) fix(lsp): respect diagnostic tag client capabilities (ocaml#909) refactor: related information in diagnostics (ocaml#907) test: update to show related information (ocaml#906) fix: offer related information only when supported (ocaml#905)
@Khady since the PR is approved you can squash & merge at your convenience. |
Thanks to both of you for the help! |
CHANGES: ## Fixes - Fix document syncing for ranges that span an entire line (ocaml/ocaml-lsp#927) - Respect the client's completion item resolve and preSelect capabilities (ocaml/ocaml-lsp#925, ocaml/ocaml-lsp#936) - Disable polling for dune's watch mode on Windows and OCaml 4.14.0 (ocaml/ocaml-lsp#935) ## Features - Semantic highlighting support is enabled by default (ocaml/ocaml-lsp#933) - Re-enable `ocamlformat-rpc` for formatting code snippets (but not files) (ocaml/ocaml-lsp#920, ocaml/ocaml-lsp#939) One needs to have either `ocamlformat` version > 0.21.0 or, otherwise, `ocamlformat-rpc` package installed. - Add custom ocamllsp/hoverExtended request (ocaml/ocaml-lsp#561) - Support utf-8 position encoding clients (ocaml/ocaml-lsp#919) - Upgrade to merlin 4.7 and use merlin's `verbosity=smart` by default, which allows unwrapping module alias types (ocaml/ocaml-lsp#942) ## Fixes - Fix semantic highlighting of "long identifiers," e.g., `Foo.Bar.x` (ocaml/ocaml-lsp#932)
CHANGES: ## Fixes - Fix document syncing for ranges that span an entire line (ocaml/ocaml-lsp#927) - Respect the client's completion item resolve and preSelect capabilities (ocaml/ocaml-lsp#925, ocaml/ocaml-lsp#936) - Disable polling for dune's watch mode on Windows and OCaml 4.14.0 (ocaml/ocaml-lsp#935) ## Features - Semantic highlighting support is enabled by default (ocaml/ocaml-lsp#933) - Re-enable `ocamlformat-rpc` for formatting code snippets (but not files) (ocaml/ocaml-lsp#920, ocaml/ocaml-lsp#939) One needs to have either `ocamlformat` version > 0.21.0 or, otherwise, `ocamlformat-rpc` package installed. - Add custom ocamllsp/hoverExtended request (ocaml/ocaml-lsp#561) - Support utf-8 position encoding clients (ocaml/ocaml-lsp#919) - Upgrade to merlin 4.7 and use merlin's `verbosity=smart` by default, which allows unwrapping module alias types (ocaml/ocaml-lsp#942) ## Fixes - Fix semantic highlighting of "long identifiers," e.g., `Foo.Bar.x` (ocaml/ocaml-lsp#932)
CHANGES: ## Features - Add support for OCaml 5.0 - Semantic highlighting support is enabled by default (ocaml/ocaml-lsp#933) - Re-enable `ocamlformat-rpc` for formatting code snippets (but not files) (ocaml/ocaml-lsp#920, ocaml/ocaml-lsp#939) One needs to have either `ocamlformat` version > 0.21.0 or, otherwise, `ocamlformat-rpc` package installed. - Add custom ocamllsp/hoverExtended request (ocaml/ocaml-lsp#561) - Support utf-8 position encoding clients (ocaml/ocaml-lsp#919) - Upgrade to merlin 4.7 and use merlin's `verbosity=smart` by default, which allows unwrapping module alias types (ocaml/ocaml-lsp#942) ## Fixes - Fix document syncing for ranges that span an entire line (ocaml/ocaml-lsp#927) - Respect the client's completion item resolve and preSelect capabilities (ocaml/ocaml-lsp#925, ocaml/ocaml-lsp#936) - Disable polling for dune's watch mode on Windows and OCaml 4.14.0 (ocaml/ocaml-lsp#935) - Fix semantic highlighting of "long identifiers," e.g., `Foo.Bar.x` (ocaml/ocaml-lsp#932)
CHANGES: ## Features - Enable [semantic highlighting](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_semanticTokens) support by default (ocaml/ocaml-lsp#933) - Support connecting over pipes and socket. Pipes on Windows aren't yet supported (ocaml/ocaml-lsp#946) [More](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#implementationConsiderations) about communication channels in LSP specification. - Re-enable `ocamlformat-rpc` for formatting code snippets (but not files and not on Windows) (ocaml/ocaml-lsp#920, ocaml/ocaml-lsp#939) One needs to have installed either `ocamlformat` package version > 0.21.0 or, otherwise, `ocamlformat-rpc` package. Note that previously `ocamlformat-rpc` came in a standalone OPAM package, but since `ocamlformat` version > 0.21.0, it comes within `ocamlformat` package. - Add custom [`ocamllsp/hoverExtended`](https://github.com/ocaml/ocaml-lsp/blob/e165f6a3962c356adc7364b9ca71788e93489dd0/ocaml-lsp-server/docs/ocamllsp/hoverExtended-spec.md#L1) request (ocaml/ocaml-lsp#561) - Support utf-8 position encoding clients (ocaml/ocaml-lsp#919) [More](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#position) about position encoding in LSP specification. - Show unwrapped module alias types on hovering over module names. This is due to upgrading to merlin 4.7 and using merlin's `verbosity=smart` by default (ocaml/ocaml-lsp#942) ## Fixes - Respect the client's completion item resolve and preSelect capabilities (ocaml/ocaml-lsp#925, ocaml/ocaml-lsp#936) - Disable polling for dune's watch mode on Windows and OCaml 4.14.0 (ocaml/ocaml-lsp#935) - Fix semantic highlighting of "long identifiers," e.g., `Foo.Bar.x` (ocaml/ocaml-lsp#932) - Fix syncing of document contents: - For ranges that span an entire line (ocaml/ocaml-lsp#927) - Previously, whole line edits would incorrectly eat the newline characters (ocaml/ocaml-lsp#971)
CHANGES: ## Features - Add support for OCaml 5.0 - Enable [semantic highlighting](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_semanticTokens) support by default (ocaml/ocaml-lsp#933) - Support connecting over pipes and socket. Pipes on Windows aren't yet supported (ocaml/ocaml-lsp#946) [More](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#implementationConsiderations) about communication channels in LSP specification. - Re-enable `ocamlformat-rpc` for formatting code snippets (but not files and not on Windows) (ocaml/ocaml-lsp#920, ocaml/ocaml-lsp#939) One needs to have installed either `ocamlformat` package version > 0.21.0 or, otherwise, `ocamlformat-rpc` package. Note that previously `ocamlformat-rpc` came in a standalone OPAM package, but since `ocamlformat` version > 0.21.0, it comes within `ocamlformat` package. - Add custom [`ocamllsp/hoverExtended`](https://github.com/ocaml/ocaml-lsp/blob/e165f6a3962c356adc7364b9ca71788e93489dd0/ocaml-lsp-server/docs/ocamllsp/hoverExtended-spec.md#L1) request (ocaml/ocaml-lsp#561) - Support utf-8 position encoding clients (ocaml/ocaml-lsp#919) [More](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#position) about position encoding in LSP specification. - Show unwrapped module alias types on hovering over module names. This is due to upgrading to merlin 4.7 and using merlin's `verbosity=smart` by default (ocaml/ocaml-lsp#942) ## Fixes - Respect the client's completion item resolve and preSelect capabilities (ocaml/ocaml-lsp#925, ocaml/ocaml-lsp#936) - Disable polling for dune's watch mode on Windows and OCaml 4.14.0 (ocaml/ocaml-lsp#935) - Fix semantic highlighting of "long identifiers," e.g., `Foo.Bar.x` (ocaml/ocaml-lsp#932) - Fix syncing of document contents: - For ranges that span an entire line (ocaml/ocaml-lsp#927) - Previously, whole line edits would incorrectly eat the newline characters (ocaml/ocaml-lsp#971)
In case someone is wondering how to call this with nvim's lsp: vim.keymap.set("n", "<f2>", function()
local params = vim.lsp.util.make_position_params()
params["verbosity"] = 1
vim.lsp.buf_request(0, "ocamllsp/hoverExtended", params, vim.lsp.handlers.hover)
end, bufopts) |
This is a proposal to emulate merlin's behavior when multiple hover commands are received in a row on a single position. Each new call should increase the verbosity counter by 1. The PR is not ready. But I'd like to know if the idea looks fine before to invest more time on it.
https://discuss.ocaml.org/t/merlin-vs-ocaml-lsp/8887
This work is sponsored by Ahrefs