-
Notifications
You must be signed in to change notification settings - Fork 459
Various refactorings in the RPC system #12580
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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