-
Notifications
You must be signed in to change notification settings - Fork 121
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
Documentation custom query #1336
Merged
Merged
Changes from 20 commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
89d3c54
add changes.md entry
PizieDust 9e42e82
add documentation spec
PizieDust c4e746d
custom documentation query implementation
PizieDust 5e8fa76
add to server capabilities
PizieDust 3fff022
unit tests
PizieDust 18394fe
add link to documentation and PR id
PizieDust cc8ac40
remove unnessary methods
PizieDust 7e43f39
make contentFormat non-optional in GetDocClientCapabilities
PizieDust 134a4ef
refactor according to code review
PizieDust 27c72db
shadow list module in test
PizieDust 9455a96
add type
PizieDust fa30962
lint
PizieDust 885e037
more test
PizieDust eeb595e
Merge branch 'master' of github.com:PizieDust/ocaml-lsp into document…
PizieDust 70f0941
expose req_document
PizieDust 938163d
refactor according to code review. expose custom getDoc
PizieDust b27a97d
linting
PizieDust 2350109
refactor according to code reveiw
PizieDust e1afe61
lint
PizieDust efb32a6
remove redundant function
PizieDust 6e6f361
update doc
PizieDust 6947e5b
better documentation
PizieDust 6018e9c
format markdown
PizieDust File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
# Documentation Request | ||
|
||
## Description | ||
|
||
This custom request allows `odoc` documentation to be gotten without using Hover. | ||
|
||
## Client capability | ||
|
||
```js | ||
export interface GetDocClientCapabilities { | ||
contentFormat: MarkupKind[]; | ||
} | ||
``` | ||
- `contentFormat`: Client supports the following content formats if the content property refers to a `literal of type MarkupContent`. The order describes the preferred format of the client. | ||
|
||
## Server capability | ||
|
||
- property name: `handleDocumentation` | ||
- property type: `boolean` | ||
|
||
|
||
## Request | ||
|
||
```js | ||
export interface GetDocParams extends TextDocumentPositionParams | ||
{ | ||
identifier?: string; | ||
contentFormat?:MarkupKind; | ||
} | ||
``` | ||
- method : `ocamllsp/getDocumentation` | ||
- params : | ||
- `position`: The position of the cursor. | ||
- `identifier`: An optional identifier. If provided, documentation for this ident is looked up from the environment at the given position. Else the server will look for the documentation of the identifier under the cursor. | ||
- `contentFormat`: Optionally override the result's format. Could be `Plaintext` or `Markdown`. | ||
|
||
## Response | ||
|
||
```js | ||
result: GetDoc | null | ||
export interface GetDoc { | ||
doc: MarkupContent; | ||
} | ||
|
||
``` | ||
- `doc`: The documentation found | ||
- A response with null result is returned if the identifier doesn't have documentation. | ||
- An error is returned if the identifier is invalid. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
112 changes: 112 additions & 0 deletions
112
ocaml-lsp-server/src/custom_requests/req_get_documentation.ml
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,112 @@ | ||
open Import | ||
module TextDocumentPositionParams = Lsp.Types.TextDocumentPositionParams | ||
|
||
let meth = "ocamllsp/getDocumentation" | ||
let capability = "handleGetDocumentation", `Bool true | ||
|
||
module GetDocClientCapabilities = struct | ||
type _t = { contentFormat : MarkupKind.t list } | ||
|
||
let _yojson_of_t { contentFormat } = | ||
`Assoc | ||
(`List (List.map ~f:(fun format -> MarkupKind.yojson_of_t format) contentFormat)) | ||
;; | ||
end | ||
|
||
module GetDocParams = struct | ||
type t = | ||
{ text_document : TextDocumentIdentifier.t | ||
; position : Position.t | ||
; identifier : string option | ||
; contentFormat : MarkupKind.t option | ||
} | ||
|
||
let t_of_yojson json = | ||
let open Yojson.Safe.Util in | ||
let textDocumentPosition = Lsp.Types.TextDocumentPositionParams.t_of_yojson json in | ||
let identifier = json |> member "identifier" |> to_option to_string in | ||
let contentFormat = | ||
json |> member "contentFormat" |> to_option MarkupKind.t_of_yojson | ||
in | ||
{ position = textDocumentPosition.position | ||
; text_document = textDocumentPosition.textDocument | ||
; identifier | ||
; contentFormat | ||
} | ||
;; | ||
|
||
let yojson_of_t { text_document; position; identifier; contentFormat } = | ||
let identifier = | ||
match identifier with | ||
| Some ident -> [ "identifier", `String ident ] | ||
| None -> [] | ||
in | ||
let contentFormat = | ||
match contentFormat with | ||
| Some format -> [ "contentFormat", MarkupKind.yojson_of_t format ] | ||
| None -> [] | ||
in | ||
`Assoc | ||
((("textDocument", TextDocumentIdentifier.yojson_of_t text_document) | ||
:: ("position", Position.yojson_of_t position) | ||
:: identifier) | ||
@ contentFormat) | ||
;; | ||
end | ||
|
||
module GetDoc = struct | ||
type t = { doc : MarkupContent.t } | ||
|
||
let yojson_of_t { doc } = `Assoc [ "doc", MarkupContent.yojson_of_t doc ] | ||
|
||
let t_of_yojson json = | ||
let open Yojson.Safe.Util in | ||
let doc = json |> member "doc" |> MarkupContent.t_of_yojson in | ||
{ doc } | ||
;; | ||
|
||
let create ~kind ~value = MarkupContent.create ~kind ~value | ||
PizieDust marked this conversation as resolved.
Show resolved
Hide resolved
|
||
end | ||
|
||
type t = GetDoc.t | ||
|
||
let t_of_yojson json = GetDoc.t_of_yojson json | ||
|
||
module Request_params = struct | ||
type t = GetDocParams.t | ||
|
||
let yojson_of_t t = GetDocParams.yojson_of_t t | ||
|
||
let create ~text_document ~position ?(identifier = None) ?(contentFormat = None) () : t = | ||
{ text_document; identifier; contentFormat; position } | ||
;; | ||
end | ||
|
||
let dispatch ~merlin ~position ~identifier ~contentFormat = | ||
Document.Merlin.with_pipeline_exn merlin (fun pipeline -> | ||
let position = Position.logical position in | ||
let query = Query_protocol.Document (identifier, position) in | ||
let result = Query_commands.dispatch pipeline query in | ||
match result with | ||
| `No_documentation | `Invalid_context | `Not_found _ -> `Null | ||
| `Builtin value | `File_not_found value | `Found value | `Not_in_env value -> | ||
let kind = | ||
match contentFormat with | ||
| Some format -> format | ||
| None -> MarkupKind.PlainText | ||
in | ||
GetDoc.yojson_of_t { doc = GetDoc.create ~kind ~value }) | ||
;; | ||
|
||
let on_request ~params state = | ||
Fiber.of_thunk (fun () -> | ||
let params = (Option.value ~default:(`Assoc []) params :> Yojson.Safe.t) in | ||
let GetDocParams.{ text_document; position; identifier; contentFormat } = | ||
GetDocParams.t_of_yojson params | ||
in | ||
let uri = text_document.uri in | ||
let doc = Document_store.get state.State.store uri in | ||
match Document.kind doc with | ||
| `Other -> Fiber.return `Null | ||
| `Merlin merlin -> dispatch ~merlin ~position ~identifier ~contentFormat) | ||
;; |
22 changes: 22 additions & 0 deletions
22
ocaml-lsp-server/src/custom_requests/req_get_documentation.mli
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
open Import | ||
|
||
module Request_params : sig | ||
type t | ||
|
||
val yojson_of_t : t -> Json.t | ||
|
||
val create | ||
: text_document:TextDocumentIdentifier.t | ||
-> position:Position.t | ||
-> ?identifier:string option | ||
-> ?contentFormat:MarkupKind.t option | ||
-> unit | ||
-> t | ||
end | ||
|
||
type t | ||
|
||
val t_of_yojson : Json.t -> t | ||
val meth : string | ||
val capability : string * [> `Bool of bool ] | ||
val on_request : params:Jsonrpc.Structured.t option -> State.t -> Json.t Fiber.t |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,163 @@ | ||
open Test.Import | ||
module Req = Ocaml_lsp_server.Custom_request.Get_documentation | ||
|
||
module Util = struct | ||
let call_documentation ~position ?(identifier = None) ?(contentFormat = None) client = | ||
let uri = DocumentUri.of_path "test.ml" in | ||
let text_document = TextDocumentIdentifier.create ~uri in | ||
let params = | ||
Req.Request_params.create ~text_document ~position ~identifier ~contentFormat () | ||
|> Req.Request_params.yojson_of_t | ||
|> Jsonrpc.Structured.t_of_yojson | ||
|> Option.some | ||
in | ||
let req = | ||
Lsp.Client_request.UnknownRequest { meth = "ocamllsp/getDocumentation"; params } | ||
in | ||
Client.request client req | ||
;; | ||
|
||
let test ~line ~character ?identifier ?contentFormat source = | ||
let position = Position.create ~character ~line in | ||
let contentFormat = | ||
match contentFormat with | ||
| Some "markdown" -> Some MarkupKind.Markdown | ||
| Some "plaintext" | _ -> Some MarkupKind.PlainText | ||
in | ||
let request client = | ||
let open Fiber.O in | ||
let+ response = call_documentation ~position ~identifier ~contentFormat client in | ||
Test.print_result response | ||
in | ||
Helpers.test source request | ||
;; | ||
end | ||
|
||
let%expect_test "Documentation of simple type with no contentFormat and no identifier" = | ||
let source = "type tree (** This is a comment *)" in | ||
let line = 0 in | ||
let character = 7 in | ||
Util.test ~line ~character source; | ||
[%expect {| { "doc": { "kind": "plaintext", "value": "This is a comment" } } |}] | ||
;; | ||
|
||
let%expect_test "Documentation of simple type with contentFormat set to markdown" = | ||
let source = "type tree (** This is another comment *)" in | ||
let line = 0 in | ||
let character = 7 in | ||
let contentFormat = "markdown" in | ||
Util.test ~line ~character ~contentFormat source; | ||
[%expect {| { "doc": { "kind": "markdown", "value": "This is another comment" } } |}] | ||
;; | ||
|
||
let%expect_test "Documentation of simple type with an identifier and contentFormat" = | ||
let source = | ||
"{|type tree (** This is another comment *)\n List.iter ~f:(fun x -> x*x) [2;4]|}" | ||
in | ||
let line = 0 in | ||
let character = 7 in | ||
let identifier = "List" in | ||
let contentFormat = "markdown" in | ||
Util.test ~line ~character ~identifier ~contentFormat source; | ||
[%expect | ||
{| | ||
{ | ||
"doc": { | ||
"kind": "markdown", | ||
"value": "List operations.\n\n Some functions are flagged as not tail-recursive. A tail-recursive\n function uses constant stack space, while a non-tail-recursive function\n uses stack space proportional to the length of its list argument, which\n can be a problem with very long lists. When the function takes several\n list arguments, an approximate formula giving stack usage (in some\n unspecified constant unit) is shown in parentheses.\n\n The above considerations can usually be ignored if your lists are not\n longer than about 10000 elements.\n\n The labeled version of this module can be used as described in the\n {!StdLabels} module." | ||
} | ||
} |}] | ||
;; | ||
|
||
let%expect_test "Documentation of simple type with an identifier and no contentFormat" = | ||
let source = | ||
"{|type tree (** This is another comment *)\n List.iter ~f:(fun x -> x*x) [2;4]|}" | ||
in | ||
let line = 0 in | ||
let character = 7 in | ||
let identifier = "List" in | ||
Util.test ~line ~character ~identifier source; | ||
[%expect | ||
{| | ||
{ | ||
"doc": { | ||
"kind": "plaintext", | ||
"value": "List operations.\n\n Some functions are flagged as not tail-recursive. A tail-recursive\n function uses constant stack space, while a non-tail-recursive function\n uses stack space proportional to the length of its list argument, which\n can be a problem with very long lists. When the function takes several\n list arguments, an approximate formula giving stack usage (in some\n unspecified constant unit) is shown in parentheses.\n\n The above considerations can usually be ignored if your lists are not\n longer than about 10000 elements.\n\n The labeled version of this module can be used as described in the\n {!StdLabels} module." | ||
} | ||
} |}] | ||
xvw marked this conversation as resolved.
Show resolved
Hide resolved
|
||
;; | ||
|
||
let%expect_test "Documentation when List module is shadowed" = | ||
let source = | ||
"{|\n\ | ||
List.iter ~f:(fun x -> x*x) [2;4]\n\ | ||
\ module List = struct\n\ | ||
\ (** This is my custom list module *)\n\ | ||
\ let rec iter ~f = function (** This is the custom iter module *)\n\ | ||
\ | [] -> () (** This is when the list is empty *)\n\ | ||
\ | x :: xs -> f x; iter ~f xs\n\ | ||
end\n\ | ||
List.iter ~f:(fun x -> x*x) [2;4]\n\ | ||
|}" | ||
in | ||
let line = 2 in | ||
let character = 6 in | ||
let identifier = "List.iter" in | ||
Util.test ~line ~character ~identifier source; | ||
[%expect | ||
{| | ||
{ | ||
"doc": { | ||
"kind": "plaintext", | ||
"value": "[iter f [a1; ...; an]] applies function [f] in turn to\n [a1; ...; an]. It is equivalent to\n [begin f a1; f a2; ...; f an; () end]." | ||
} | ||
} |}] | ||
;; | ||
|
||
let%expect_test "Documentation when List module is shadowed" = | ||
let source = | ||
"{|\n\ | ||
List.iter ~f:(fun x -> x*x) [2;4]\n\ | ||
module List = struct\n\ | ||
\ (** This is my custom list module *)\n\ | ||
\ let rec iter ~f = function (** This is the custom iter module *)\n\ | ||
\ | [] -> () (** This is when the list is empty *)\n\ | ||
\ | x :: xs -> f x; iter ~f xs\n\ | ||
end\n\ | ||
Base.List.iter ~f:(fun x -> x*x) [2;4]\n\ | ||
|}" | ||
in | ||
let line = 7 in | ||
let character = 12 in | ||
let identifier = "Base.List.iter" in | ||
Util.test ~line ~character ~identifier source; | ||
[%expect {| | ||
{ "doc": { "kind": "plaintext", "value": "Base.List.iter" } } |}] | ||
;; | ||
|
||
(* TODO: Open Issue in Merlin to investigate while this doesnt return documentation of the custom List module*) | ||
let%expect_test "Documentation when List module is shadowed" = | ||
let source = | ||
"{|\n\ | ||
List.iter ~f:(fun x -> x*x) [2;4]\n\ | ||
module List = struct\n\ | ||
\ (** This is my custom list module *)\n\ | ||
\ let rec iter ~f = function (** This is the custom iter module *)\n\ | ||
\ | [] -> () (** This is when the list is empty *)\n\ | ||
\ | x :: xs -> f x; iter ~f xs\n\ | ||
end\n\ | ||
Base.List.iter ~f:(fun x -> x*x) [2;4]\n\ | ||
|}" | ||
in | ||
let line = 2 in | ||
let character = 9 in | ||
Util.test ~line ~character source; | ||
[%expect | ||
{| | ||
{ | ||
"doc": { | ||
"kind": "plaintext", | ||
"value": "List operations.\n\n Some functions are flagged as not tail-recursive. A tail-recursive\n function uses constant stack space, while a non-tail-recursive function\n uses stack space proportional to the length of its list argument, which\n can be a problem with very long lists. When the function takes several\n list arguments, an approximate formula giving stack usage (in some\n unspecified constant unit) is shown in parentheses.\n\n The above considerations can usually be ignored if your lists are not\n longer than about 10000 elements.\n\n The labeled version of this module can be used as described in the\n {!StdLabels} module." | ||
} | ||
} |}] | ||
;; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -61,6 +61,7 @@ | |
syntax_doc_tests | ||
test | ||
type_enclosing | ||
documentation | ||
with_pp | ||
with_ppx | ||
workspace_change_config)))) |
Oops, something went wrong.
Oops, something went wrong.
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.
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.
position
does not appear in the object type above - is that intentional?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.
Yes, since the interface itself extends
TextDocumentPositionParams
which contains both the Document Uri and the PositionThere 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 can probably provide a link to the TextDocumentPositionParams and remove the description of
position
(sincetextDocument
, theTextDocumentIdentifier
is not mentionned in the documentation).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.
Not sure that it is right. I think that you should explain that the param should be a TextDocumentPositionParams (with a link to the specification) with additonal ones, enumerated.
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.
@xvw I just pushed a commit with some documentation changes. Can you check if it's more structured?