Some ideas for improvement #19
Conversation
|
This is how the user API changes: dpc/rust-bitcoin-rpc@dpc-dev...dpc:dpc-dev-jsonrpc-reorg |
|
I like this, though I wonder if it's possible to go farther. At Blockstream we have a similar wrapper to your
|
Both of these fit with my general plan to make |
What can be taken by reference, is taken by reference. This gives `Request` a lifetime, so it can't derive `Deserialize`. Because of that, I had to delete a test, but that seems OK, since it looks like this test is testing `serde_json` crate, and not any logic in this crate.
src/client.rs
Outdated
| let request = self.build_request(rpc_name, args); | ||
| let response = self.send_request(&request)?; | ||
|
|
||
| Ok(response.clone().into_result()?) |
There was a problem hiding this comment.
Why is this clone required here?
|
ACK |
src/client.rs
Outdated
| @@ -114,14 +113,14 @@ impl Client { | |||
| } | |||
|
|
|||
| /// Builds a request | |||
| pub fn build_request(&self, name: String, params: Vec<serde_json::Value>) -> Request { | |||
| pub fn build_request<'a>(&self, name: &'a str, params: &'a [serde_json::Value]) -> Request<'a> { | |||
There was a problem hiding this comment.
Can you split this into two lifetimes, one for name and one for params?
|
ACK |
While
RequestandResponsemight be useful, for a lot of use-cases they are just an implementation detail, that just gets in the way. I was wondering if they could be made non-private altogether, but I guess for some advanced uses, they might be necessary.So the first patch makes some allocation savings, and second add a convenience function that allows forgetting that
ResponseandRequestexist at all. First patch have a longer commit explaining more about what's going on.