Various refactorings in the RPC system#12580
Conversation
|
For 1, could you split it off into a separate PR. That should be easy to review. You can also rename the files there too. The rest I will need to think a bit longer about. |
shonfeder
left a comment
There was a problem hiding this comment.
I think the core of this change suggestion points towards a useful cleanup. However, I am doubtful that introducing the new GADT is the most ergonomic route, so I've suggested a simpler approach, based on introducing a function for sending notifications.
In case you agree with my reasoning, you can see or must merge in mu suggestion from 0129cee
I am unsure whether removing the result that represents RPC message sending errors is wise, but I am not well situated to know whether the complexity of that explicit result value is worth the overhead, so will leave for you and others to decide.
| val wrap_build_outcome_exn | ||
| : print_on_success:bool | ||
| -> ('a | ||
| -> (Dune_rpc.Build_outcome_with_diagnostics.t, Dune_rpc.Response.Error.t) result | ||
| Fiber.t) | ||
| -> 'a | ||
| -> Dune_rpc.Build_outcome_with_diagnostics.t |
There was a problem hiding this comment.
I see we don't always use this when we call fire_*. Do you know why/when it is needed and where not? If so, that would be helpful info to add to the interface as documentation.
There was a problem hiding this comment.
The new type should make the decision to use or not to use this function clearer:
Build_outcome_with_diagnostics.t -> unit
It just unwraps the sum type and prints errors and so. Don't use it if you want to print different stuff
| -> ('a, 'b) Dune_rpc.Decl.request | ||
| -> ('a, 'b) message_kind | ||
| -> 'a | ||
| -> ('b, Dune_rpc.Response.Error.t) result Fiber.t |
There was a problem hiding this comment.
How come we don't want to know whether the RPC resulted in error anymore? It seems to me that it would be useful, as I can imagine we could recover from sending requests in some cases.
There was a problem hiding this comment.
It's a pragmatic change: every single user of this function had the exact same error handling code, so I just factored it in directly.
c264b31 to
be3f528
Compare
be3f528 to
3153b5a
Compare
4fbf40c to
c00993f
Compare
|
As a learning exercise, I'm using https://github.com/jj-vcs/jj to split this into several smaller PRs, thanks @Alizter :) |
|
Is this ready for review, modulo resolving conflicts, or does the latest comment about playing with JJ indicate that you are planning to replace this with different PRs? |
|
@shonfeder There are quite a few unrelated changes in this PR, so @ElectreAAS said she would split them off slowly. I will mark this as draft until we are done with it in which case we can close. |
Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>
Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>
Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>
Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>
This reverts commit be01cb9. Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>
This produces a simpler API w/r/t language surface complexity, and to the complexity users need to consider when sending RPC messages. The cost is a few lines of code duplication. Signed-off-by: Shon Feder <shon.feder@gmail.com>
Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>
Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>
c00993f to
5be0c8b
Compare
This PR is a spin-off of #12473, containing only refactors. I tried to make each commit one single idea, independant of the others, but since they all modify the same files they're not git-independant.
1. Simple renaming of modulesHas been moved to #12585, is reverted in commit 6.Rpc.Rpc_stuff -> Rpc.Stuff(but not filenames)2.
rpc_build.mlandbin/build.mlcontained functions that were just thin wrappers around functionnalities inrpc_common. I removed said wrappers, so everyone has the same API.3.
Rpc.Common.run_via_rpcwas also just a thin wrapper around two functions in the same module, it is deleted.wrap_build_outcome_exndidn't need to be a higher order function. The error handling is also unified across users of the API.4. Having a nicer API means
diagnostics.mlcan just use it, no questions asked.5. To do the same as in 4 to
shutdown.ml, I needed to abstract the message kind (request or notification), and this is done viaGADTtwo functions that are cleaner nowFeel free to suggest further PR splitting if necessary