Toplevel rpc #159
Toplevel rpc #159
Conversation
The IDL is defined in `toplevel_api.ml`, and both client and server are autogenerated from this. Client currently uses the Lwt generator and the server is synchronous.
jonludlam
force-pushed
the
toplevel-rpc
branch
from
deaad88
to
fe555bf
Compare
There was a problem hiding this comment.
Looks really great! Definitely once you get your head around ocaml-rpc it tidies up a substantial amount of the code and as @tmattio pointed out offline, should make it quite portable if we ever need to change the underlying transport mechanism. One small change (with a seemingly big impact) and this should be good to go I think. Thanks :))
Pointed out by @patricoferris
jonludlam
force-pushed
the
toplevel-rpc
branch
from
17dadf2
to
92bf105
Compare
|
|
||
| module M = Idl.IdM (* Identity monad - server is synchronous *) | ||
|
|
||
| module IdlM = Idl.Make (M) |
There was a problem hiding this comment.
Can we call it Identity or Rpc_identity? It'd make the code easier to read I think
There was a problem hiding this comment.
Hrm, perhaps I should just get rid of the comment - it's not strictly the identity monad ;-) (it boxes the results because of lack of injectivity annotation in the past). The important part of the comment is that we're using the synchronous module rather than the Lwt one.
| "rpclib" | ||
| "rpclib-lwt" | ||
| "ppx_deriving_rpc" |
There was a problem hiding this comment.
We need to add these to the dune-project deps
| module Toprpc = Toplevel_api.Make (Rpc_lwt.GenClient ()) | ||
|
|
||
| (* Handy infix bind-style operator for the RPCs *) | ||
| let ( >>>= ) x f = |
There was a problem hiding this comment.
We can define a let binding here to stay consistent with the rest of the code
src/ocamlorg_toplevel/bin/worker.ml
Outdated
| let complete phrase = | ||
| let n, res = UTop_complete.complete ~phrase_terminator:";;" ~input:phrase in | ||
| let res = | ||
| List.filter (fun (l, _) -> not (Astring.String.is_infix ~affix:"__" l)) res |
There was a problem hiding this comment.
We could define is_infix with String.sub here to avoid adding the Astring library (even if it's already a transitive dep)
There was a problem hiding this comment.
I'll use the function we have in odoc.
src/ocamlorg_toplevel/bin/worker.ml
Outdated
| IdlM.ErrM.return Toplevel_api.{ n; completions } | ||
|
|
||
| let server process e = | ||
| let ( >>= ) = M.bind in |
There was a problem hiding this comment.
Same as above, we can use a let binding
| @@ -0,0 +1,94 @@ | |||
| (** IDL for talking to the toplevel webworker *) | |||
There was a problem hiding this comment.
Can we put the docstrings in an interface file?
| @@ -0,0 +1,49 @@ | |||
| (** Worker rpc *) | |||
There was a problem hiding this comment.
Same as above, can we create an interface for the modules' documentation?
There was a problem hiding this comment.
Oops, you're right for worker_rpc.ml - it's the IDL that should keep the comments in the .ml file
jonludlam
force-pushed
the
toplevel-rpc
branch
from
c1b47fb
to
c744dc1
Compare
|
Thanks for the review @tmattio - I've addressed most of the comments. In adding the [.mli] for [Worker_rpc] I realised our handling of timeouts wasn't particularly good so I've improved that too. |
Spotted by @tmattio.
jonludlam
force-pushed
the
toplevel-rpc
branch
from
c744dc1
to
f407ff3
Compare
|
Let's get this in. |
This is on top of the completion PR. Personally I think this looks tidier, but I appreciate there's some extra cognitive load due to the RPC definitions. The RPC library is inspired by
ctypesso anyone who's done C bindings with that might have an easier time with the IDL file.