Signature Help #1720
Signature Help #1720
Conversation
|
@voodoos, I've rebased this branch and it should be good for review |
|
Thanks Rafal ! |
|
|
||
| let active_parameter_by_arg ~arg params = | ||
| let find_by_arg = function | ||
| | { argument = Some a; _ } when a == arg -> true |
There was a problem hiding this comment.
Not particularly familiar with the structure of a, but do we really want physical equality (rather than structural equality?).
| | _ -> None | ||
|
|
||
| module String = struct | ||
| include String |
There was a problem hiding this comment.
Is it necessary to include String (as you can always reference Stdlib.String if necessary) and inclusion doesn't seem to be useful "within the current String module"?
There was a problem hiding this comment.
We could also move these utilities to Merlin's Std.
| let lsplit2 s ~on = | ||
| match String.index_opt s on with | ||
| | None -> None | ||
| | Some i -> | ||
| Some (sub s ~pos:0 ~len:i, sub s ~pos:(i + 1) ~len:(length s - i - 1)) |
There was a problem hiding this comment.
WDYT about using Option.map here?
| let pos = | ||
| match pos with | ||
| | None -> 0 | ||
| | Some pos -> pos + 1 | ||
| in |
There was a problem hiding this comment.
Maybe let pos = (Option.value ~default:(-1) pos) + 1 ?
| val application_signature : | ||
| prefix:string | ||
| -> Mbrowse.t | ||
| -> application_signature option | ||
|
|
||
| val prefix_of_position : | ||
| short_path: bool | ||
| -> Msource.t | ||
| -> Msource.position | ||
| -> string |
There was a problem hiding this comment.
WDYT about moving (or duplicating) some of your meaningful comments from the .ml to the .mli as doc purpose?
There was a problem hiding this comment.
Thanks Rafal, I think this covers well the first step which was to bring the code from ocaml-lsp to merlin-lib.
You should also document the new command in the protocol file: https://github.com/ocaml/merlin/blob/master/doc/dev/PROTOCOL.md
Aditionnaly, it looks like the LSP protocol also provides additional optional parameters to help the server provide a correct answer: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_signatureHelp
May be we should also accept these even if they are not used right now ?
The goal of this PR is to upstream SignatureHelp functionality from ocaml-lsp to merlin. Similarly to #1286.