fringe depot
Hey all, I'd like to get some feedback on this code I wrote.
I'm especially interested in getting some feedback for making the wait_for function more ergonomic and removing the duplicated structs of ResponseType and Response.
I was considering turning wait_for into a macro but I'm unsure if that's the best solution.
here's how the api is supposed to be used:
let client = DebugClient::new();
// Initialize
let init_resp = Request::new()
.command("initialize")
.arg("adapterID", "python".into())
.send(&mut client)
.await;
dbg!(init_resp);
// Launch
let launch_resp = Request::new()
.command("launch")
.wait_for(Some(Response::Event("initialized".to_string())))
.arg("code", "x = 1;print(x);".into())
.send(&mut client)
.await;
dbg!(launch_resp);
// sending the same request twice
let mut some_req = Request::new();
some_req.command("").arg("t", "t".into());
let resp1 = some_req.send(&mut client).await;
let resp2 = some_req.send(&mut client).await;
realized now that I could probably give the new method a parameter of command to eliminate the need to call command on every request
fringe depot
bump
tawdry swallow
//this should always be "request"
pub r#type: String,
```Is this field only here to get serialized? Technically, you could make your typing a bit stronger by using the `serde_with` crate
```rs
//I'm assuming you also remove the `Deserialize` impl you probably don't use: When would you be deserializing a request?
//if you need it, there's a way to make that work too
#[derive(Debug, SerializeDisplay)]
struct RequestType;
impl Display for RequestType {
fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
f.write_str("request")
}
}
pub r#type: RequestType,
```This also removes the runtime cost of creating a `String`, or the possibility of doing it wrong.
```rs
pub fn arg(&mut self, key: &str, value: Value) -> &mut Self {
self.arguments
.as_object_mut()
.unwrap()
.insert(key.to_string(), value);
self
}
```If calling this function on an `arguments` that isn't an object panics, why is `arguments` stored as a `Value` and not a `Map<String, Value>`? The latter would let you ensure that this panic never happens.
```rs
pub fn args(&mut self, args: Map<String, Value>) -> &mut Self {
*self.arguments.as_object_mut().unwrap() = args;
self
}
```Since `arg` _adds_ an argument, I'd have expected `args` to _add_ multiple arguments, not _replace_ the existing ones. The implementation is fine (except for the bit where `arguments` probably shouldn't be a `Value`), but the name is a bit off IMO/// Change what will happen when a response is sent.
///
/// * If [`None`], then sending will return [`Null`](serde_json::Value::Null)
/// * If [`Some(Response::Event(e))`], then sending will wait for any event whose name matches e
/// * If [`Some(Response::Response)`], then sending will wait for the response to the request.
/// This is what happens by default
```I'm a bit confused by these docs. How can sending a request wait for the response to that same request? Do you mean that the send operation will `await` for the response to arrive and return it? Also, when would receiving an immediate `Null` be desirable? I feel like this should be a separate `send` function that returns `()` (or the same function with more generics), though I suppose the null is harmless.
The function itself is fine, though I'd be tempted to add a `Null` variant to the `Response` enum rather than wrapping it in an option, simply because `Response::Response` looks a bit cleaner than `Some(Response::Response)`.
```rs
let mut value = serde_json::to_value(&self).unwrap();
let value = value.as_object_mut().unwrap()
```This appears to be an elaborate scheme to call `self.clone()`, except slower
```rs
let value = serde_json::to_value(value).unwrap();
```And this is _literally_ `value.clone()` except slower.
I'd suggest rewriting that function in this form:
```rs
//take an owned self, so as to avoid cloning.
fn build(self, seq: u32) -> Request {
Request {
seq,
command: self.command,
arguments: self.arguments,
r#type: RequestType, //see the first suggestion
}
}
```This is also a good opportunity to bring up that most uses of the builder pattern in Rust tend to be `self -> Self` method, not `&mut self -> &mut Self`. Because the final method usually wants an owned builder so it can move out of it. If you want to clone, type `.clone()` at the call site, so callers that don't need to reuse a request builder
can avoid paying for a useless clone.Actually, on second thought, this means you don't need Serialize or Deserialize on the builder at all. Their only user was build itself.
I also have a broader point to make about this API, which is that you don't really need arg or args, or perhaps don't even need the builder. Constructing a Value is easy, there's even a nice json! macro for it. You could just make the caller directly construct a request and send that. You could even avoid using a Value at all, anywhere, by replacing it with some generic parameters. Value is an extremely annoying thing to use in practice, and is a very inefficient tool if you know at compile time what your json is shaped like.
Actually, broader point still: This API is weirdly dynamic for something that could be represented by an enum of all existing commands. Or a bunch of structs (one per command) and a generic parameter
It works, don't get me wrong, but it looks a bit too much like a python or JS API for the taste of the average Rust programmer. Most of us would rather have an API that leverages types to ensure that we don't write anything stupid like a cancel command with the correct arguments for a goto one
fringe depot
thanks for the feedback! I had kinda the same thoughts about a lot of things but I did much of this code before a lot of other things in my api so I didn't have much experience, which is why a lot of it is pretty dumb.
This is also a good opportunity to bring up that most uses of the builder pattern in Rust tend to be self -> Self method, not &mut self -> &mut Self. Because the final method usually wants an owned builder so it can move out of it.
this is good to know, I was kinda copying off ofstd::process::Commandwhich uses &mut self. I didn't like that implementation and thought about self -> Self, but I figured this must be the standard if standard library uses it. ig Command isn't really a builder so I should have tried looking at something else to model off of.
Actually, broader point still: This API is weirdly dynamic for something that could be represented by an enum of all existing commands. Or a bunch of structs (one per command) and a generic parameter
It works, don't get me wrong, but it looks a bit too much like a python or JS API for the taste of the average Rust programmer. Most of us would rather have an API that leverages types to ensure that we don't write anything stupid like a cancel command with the correct arguments for a goto one
Very good point, this was primarily something quick and dirty I tried doing to get stuff working. I decided to share it because I knew a lot of it was bad so I could learn from it. I actually found this crate that has a much more Rusty implementation, so I will be scrapping pretty much the entire request method in favor of that crate.
Thanks again for all your feedback, I found all of it extremely informative and helpful!
tawdry swallow
thanks for the feedback! I had kinda the same thoughts about a lot of things but...
Command is arguably a builder, but a slightly strange one, in that no efficiency is lost by making the final methods & ones
fringe depot
Implemented the crate and now to send a request to the debugger, instead of making a builder and doing request.send(client), the api is instead client.send(request) -> () or client.send_and_wait(request: Request, condition: WaitFor) -> Response/Event, where WaitFor is just the ResponseType enum from before:
enum WaitFor {
Response(u64),
Event(events::Event),
}
impl WaitFor {
pub fn get_condition(mut self) -> impl FnMut(&ProtocolMessageContent) -> bool {}
}
However I'm still running into an implementation issue where I don't want to let the user have access to the Response variant's data, since that is just going to be the Request's seq value. Before I had a separate enum to cover this that was the same as the WaitFor enum but the Response variant didn't have u64 data in it, so you were forced to either use WaitFor::Response or WaitFor::Event(e).
This implementation was fine but I didn't like having two separate enums. Do you think this is fine or do you have a better idea @tawdry swallow ?
tawdry swallow
Implemented the crate and now to send a request to the debugger, instead of maki...
If you want to hide the contents of Response, making two separate enums is a valid option, yes.
Technically you could also do this
pub struct ResponseId(u64); //notice the u64 is private
```and you put _that_ inside `WaitFor`, though it means users will have to produce an id if they ever need to construct that variant