Skip to content
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

Expose individual server RPC endpoints for advanced users #229

Merged
merged 4 commits into from
Jan 17, 2024

Conversation

mbarbin
Copy link
Contributor

@mbarbin mbarbin commented Jan 12, 2024

In this PR, I propose that the generated service interface exposes the
individual RPC endpoints currently hidden. Here's an example of the change this
PR would make to a generated service:

@|-1,9 +1,19 ============================================================
 |  module Server : sig
 |    (** Produce a server implementation from handlers *)
 |    val make : 
 |      get:((key, unary, value_or_error, unary) Server.rpc -> 'handler) ->
 |      set:((keyval_pair, unary, unit_, unary) Server.rpc -> 'handler) ->
 |      delete:((key, unary, unit_or_error, unary) Server.rpc -> 'handler) ->
 |      listKeys:((unit_, unary, keys, unary) Server.rpc -> 'handler) ->
 |      unit -> 'handler Pbrt_services.Server.t
+|    
+|    (** The individual server stubs are only exposed for advanced users. Casual users should prefer accessing them through {!make}. *)
+|    
+|    val get : (key,unary,value_or_error,unary) Server.rpc
+|    
+|    val set : (keyval_pair,unary,unit_,unary) Server.rpc
+|    
+|    val delete : (key,unary,unit_or_error,unary) Server.rpc
+|    
+|    val listKeys : (unit_,unary,keys,unary) Server.rpc
 |  end

This idea originated from our discussion in #224

For a motivating example for this change, you can have a look at this pull request from a personal project.

  • At present, I've maintained the "rpc_" prefix for names (e.g., "rpc_get"
    from the above example). However, I'm considering the potential for more
    consistency by using identical names for the client and server RPCs. Would
    there be any drawbacks to renaming the server endpoint to remove the "rpc_"
    prefix? Edit: now renamed to remove the prefix
  • To be rebased once Proposal for automated generation of dune diff rules in examples/ #231 is merged

Thank you for your time and consideration!

@c-cube
Copy link
Collaborator

c-cube commented Jan 12, 2024

I'm open to this, it seems reasonable. The reason why there's rpc_… as a prefix is to make sure not to shadow make (which I do intend to keep!). However I suppose the codegen could just make sure that if a RPC endpoint is called "make" it gets renamed to make_ or something in that vein (both on the client and server sides).

@mbarbin
Copy link
Contributor Author

mbarbin commented Jan 14, 2024

Thank you for pointing out the potential issue of shadowing make, which I hadn't considered. I've updated the PR to adopt your suggested make_ naming convention and included a test case for this specific scenario.

The changes are now reflected in the examples/ expect-tests.

This PR is currently built on top of #231 . If #231 231 isn't considered for merging, please inform me and I'll adjust accordingly.

Thank you for your time and guidance!

@c-cube
Copy link
Collaborator

c-cube commented Jan 15, 2024

This looks good. Is there anything you want to add before I merge?

@mbarbin mbarbin changed the title Proposal to expose individual server RPC endpoints for advanced users Expose individual server RPC endpoints for advanced users Jan 15, 2024
@mbarbin
Copy link
Contributor Author

mbarbin commented Jan 15, 2024

@c-cube Thank you for the review! I've tested the changes on my end with a preview package, and this behaves as I expected. Good to go from my perspective. Thank you!

@c-cube c-cube merged commit 67870ed into mransan:master Jan 17, 2024
2 checks passed
@mbarbin mbarbin deleted the expose-server-stubs branch January 28, 2024 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants