ocaml / ocaml-lsp

OCaml Language Server Protocol implementation
Other
749 stars 117 forks source link

Global pass on Custom_request #1346

Closed xvw closed 1 month ago

xvw commented 1 month ago

The patch proposes a global pass on Custom Requests by trying to impose the module Request_params structure, which proposes a creation function (and projection in Yojson), and a type t at the module toplevel (with a conversion function from Yojson).

I'm obviously open to feedback!

coveralls commented 1 month ago

Pull Request Test Coverage Report for Build 4447

Details


Changes Missing Coverage Covered Lines Changed/Added Lines %
ocaml-lsp-server/src/custom_requests/req_hover_extended.ml 1 2 50.0%
ocaml-lsp-server/src/custom_requests/req_wrapping_ast_node.ml 0 1 0.0%
ocaml-lsp-server/src/custom_requests/util.ml 3 7 42.86%
ocaml-lsp-server/src/custom_requests/req_typed_holes.ml 0 5 0.0%
ocaml-lsp-server/src/custom_requests/req_type_enclosing.ml 9 15 60.0%
ocaml-lsp-server/src/custom_requests/req_merlin_call_compatible.ml 15 23 65.22%
<!-- Total: 28 53 52.83% -->
Files with Coverage Reduction New Missed Lines %
ocaml-lsp-server/src/custom_requests/req_hover_extended.ml 1 51.72%
ocaml-lsp-server/src/custom_requests/req_merlin_call_compatible.ml 7 65.08%
<!-- Total: 8 -->
Totals Coverage Status
Change from base Build 4443: -0.03%
Covered Lines: 5519
Relevant Lines: 25403

💛 - Coveralls
rgrinberg commented 1 month ago

LGTM. Some further suggestions for the future:

  1. Move the declarations (request/response type definition + encoders) of the custom requests to a separate library and make a package.

  2. Introduce a type ('req, 'resp) custom_request to have a uniform API

xvw commented 1 month ago

LGTM. Some further suggestions for the future:

1. Move the declarations (request/response type definition + encoders) of the custom requests to a separate library and make a package.

2. Introduce a type `('req, 'resp) custom_request` to have a uniform API

Yes, it is on my todo-list! I was also thinking about having a Custom_request.run : ('req, 'resp) custom_request -> 'req -> 'resp to avoid relaying on run Unknown_request at the user level.

Thanks a lot for the quick review!