search

ocaml / ocaml-lsp

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

Type enclosing query #1304

Closed xvw closed 2 months ago

xvw commented 2 months ago

This PR is based on 1265 and add a type enclosing custom request.

voodoos commented 2 months ago

I don't remember: is there any reason why this is based on the 5.2 branch instead of master ?

xvw commented 2 months ago

I don't remember: is there any reason why this is based on the 5.2 branch instead of master ?

~I am currently rebased on master!~

Branch rebased on master.

coveralls commented 2 months ago

Pull Request Test Coverage Report for Build 4367

Details

Changes Missing Coverage Covered Lines Changed/Added Lines %
ocaml-lsp-server/src/custom_requests/req_type_enclosing.ml 63 89 70.79%
<!-- Total: 63 89 70.79% -->
Totals Coverage Status
Change from base Build 4363: 0.3%
Covered Lines: 5515
Relevant Lines: 24717
💛 - Coveralls
coveralls commented 2 months ago

Pull Request Test Coverage Report for Build 4368

Details

Changes Missing Coverage Covered Lines Changed/Added Lines %
ocaml-lsp-server/src/custom_requests/req_type_enclosing.ml 64 90 71.11%
<!-- Total: 64 90 71.11% -->
Totals Coverage Status
Change from base Build 4363: 0.3%
Covered Lines: 5515
Relevant Lines: 24717
💛 - Coveralls
coveralls commented 2 months ago

Pull Request Test Coverage Report for Build 4369

Details

Changes Missing Coverage Covered Lines Changed/Added Lines %
ocaml-lsp-server/src/custom_requests/req_type_enclosing.ml 63 89 70.79%
<!-- Total: 63 89 70.79% -->
Totals Coverage Status
Change from base Build 4363: 0.2%
Covered Lines: 5505
Relevant Lines: 24716
💛 - Coveralls
coveralls commented 2 months ago

Pull Request Test Coverage Report for Build 4370

Details

Changes Missing Coverage Covered Lines Changed/Added Lines %
ocaml-lsp-server/src/custom_requests/req_type_enclosing.ml 44 57 77.19%
<!-- Total: 44 57 77.19% -->
Totals Coverage Status
Change from base Build 4363: 0.2%
Covered Lines: 5485
Relevant Lines: 24684
💛 - Coveralls
coveralls commented 2 months ago

Pull Request Test Coverage Report for Build 4371

Details

Changes Missing Coverage Covered Lines Changed/Added Lines %
ocaml-lsp-server/src/custom_requests/req_type_enclosing.ml 45 57 78.95%
<!-- Total: 45 57 78.95% -->
Totals Coverage Status
Change from base Build 4363: 0.2%
Covered Lines: 5486
Relevant Lines: 24684
💛 - Coveralls
coveralls commented 2 months ago

Pull Request Test Coverage Report for Build 4372

Details

Changes Missing Coverage Covered Lines Changed/Added Lines %
ocaml-lsp-server/src/custom_requests/req_type_enclosing.ml 47 59 79.66%
<!-- Total: 47 59 79.66% -->
Totals Coverage Status
Change from base Build 4363: 0.2%
Covered Lines: 5489
Relevant Lines: 24686
💛 - Coveralls
coveralls commented 2 months ago

Pull Request Test Coverage Report for Build 4373

Details

Changes Missing Coverage Covered Lines Changed/Added Lines %
ocaml-lsp-server/src/custom_requests/req_type_enclosing.ml 47 59 79.66%
<!-- Total: 47 59 79.66% -->
Totals Coverage Status
Change from base Build 4363: 0.2%
Covered Lines: 5489
Relevant Lines: 24686
💛 - Coveralls
coveralls commented 2 months ago

Pull Request Test Coverage Report for Build 4380

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

Changes Missing Coverage Covered Lines Changed/Added Lines %
ocaml-lsp-server/src/custom_requests/req_type_enclosing.ml 51 64 79.69%
<!-- Total: 51 64 79.69% -->
Totals Coverage Status
Change from base Build 4363: 0.2%
Covered Lines: 5493
Relevant Lines: 24691
💛 - Coveralls
coveralls commented 2 months ago

Pull Request Test Coverage Report for Build 4382

Details

Changes Missing Coverage Covered Lines Changed/Added Lines %
ocaml-lsp-server/src/custom_requests/req_type_enclosing.ml 51 64 79.69%
<!-- Total: 51 64 79.69% -->
Totals Coverage Status
Change from base Build 4378: 0.2%
Covered Lines: 5493
Relevant Lines: 24691
💛 - Coveralls
coveralls commented 2 months ago

Pull Request Test Coverage Report for Build 4383

Details

Changes Missing Coverage Covered Lines Changed/Added Lines %
ocaml-lsp-server/src/custom_requests/req_type_enclosing.ml 57 70 81.43%
<!-- Total: 57 70 81.43% -->
Totals Coverage Status
Change from base Build 4378: 0.2%
Covered Lines: 5499
Relevant Lines: 24697
💛 - Coveralls
coveralls commented 2 months ago

Pull Request Test Coverage Report for Build 4392

Details

Changes Missing Coverage Covered Lines Changed/Added Lines %
ocaml-lsp-server/src/custom_requests/req_type_enclosing.ml 61 77 79.22%
<!-- Total: 61 77 79.22% -->
Totals Coverage Status
Change from base Build 4387: 0.2%
Covered Lines: 5502
Relevant Lines: 24704
💛 - Coveralls
rgrinberg commented 2 months ago

Sorry for the long winded review. Regarding all the issues with json, I realize that these are issues that are essentially present in all of our custom requests. Indeed, we've added a lot of custom code doing things that are standard and all of our standard requests have a quirky behavior. I see that at least somebody tried to unify things a little bit by introducing Custom_request. We should expand that example. To briefly summarize the rules for custom requests:

  1. The overarching rule is that hand written encoders/decoders should be interchangeable with generated ones.
  2. This means that we need to follow all the conventions of generated encoders/decoders
  3. It's good to have explicit request/response types for every single custom request (They're going to be very useful once we need to version things)
  4. The conversion from json decoding errors to the appropriate jsonrpc error should be done once and for all somewhere in jsonrpc-fiber or lsp-fiber. E.g. we handle this for all regular lsp requests in one swoop: 
      match In_request.of_jsonrpc req with
  | Error message ->
    let code = Jsonrpc.Response.Error.Code.InvalidParams in
    let error = Jsonrpc.Response.Error.make ~code ~message () in
    Fiber.return
      (Jsonrpc_fiber.Reply.now (Jsonrpc.Response.error req.id error), state)

We should strive for the same for custom requests.

In any case, it's not fair to hold your code to a higher standard to all previous code. So for now you don't need to address to make everything uniform, but I would like at least point 3 addressed with request/response module and their own t_of_yojson and yojson_of_t functions (of whatever type).

xvw commented 2 months ago

@rgrinberg

Thank you very much for your detailed review!

In any case, it's not fair to hold your code to a higher standard to all previous code.

From my point of view, it's not worth merging code that we explicitly know can be improved, so I'm going to try, as best I can, to take into account all the proposed points. Then I'll try to do a pass on all the custom requests that have already been implemented.

Thank you very much, I'll take the liberty of pinging you when this PR is ready (Monday).

rgrinberg commented 2 months ago

Sure, I'm certainly against improving things across the board. Feel free to regularize all custom requests. I'll give some background why the situation is like this in the first place. In the beginning, I was very much against custom requests and even more so against investing a lot of work into them. I thought they would require a lot duplicate work on the client side and the LSP protocol would evolve to make them all obsolete anyhow. The LSP protocol didn't really evolve much and the use cases for custom requests steadily grew. The end result is that we ended up with a lot of ad-hoc extensions.

coveralls commented 2 months ago

Pull Request Test Coverage Report for Build 4395

Details

Changes Missing Coverage Covered Lines Changed/Added Lines %
ocaml-lsp-server/src/custom_requests/req_type_enclosing.ml 57 79 72.15%
<!-- Total: 57 79 72.15% -->
Totals Coverage Status
Change from base Build 4387: 0.2%
Covered Lines: 5498
Relevant Lines: 24706
💛 - Coveralls
coveralls commented 2 months ago

Pull Request Test Coverage Report for Build 4397

Details

Changes Missing Coverage Covered Lines Changed/Added Lines %
ocaml-lsp-server/src/custom_requests/req_type_enclosing.ml 57 78 73.08%
<!-- Total: 58 79 73.42% -->
Totals Coverage Status
Change from base Build 4387: 0.2%
Covered Lines: 5499
Relevant Lines: 24706
💛 - Coveralls
voodoos commented 2 months ago

The CI failures appear to be unrelated ? Maybe we should bump the setup-ocaml action like in https://github.com/ocaml/merlin/pull/1791 ?