Hide Value from user and use serde instead #19
Conversation
|
Hey sorry for leaving this open so long, I have a hard time finding time for this project. I think I can work at it again real soon, but first I'll need to do chores I guess (CI...), so it might still need some time. Especially seeing this is somewhat complicated for me :) I'm a bit hesitating anyways, since it adds serde as a dependency, and I'm not yet sure if that's really pulling it's weight. But that strongly typed API and custom structs really are enticing! |
|
Its okay, I was working on a larger pr after I submitted this one by I lost interest. However, I do think it is very nice because then all functions can take a serde trait bound allowing for very expressive calling of functions. |
|
So I'll be putting in some inline questions for @smolck who wants to take over here. |
| @@ -96,17 +103,31 @@ where | |||
| (req, fut) | |||
| } | |||
|
|
|||
| async fn send_msg( | |||
| /// Will panic if args is serialized into something that is not an array | |||
There was a problem hiding this comment.
Where does that happen? How can the caller ensure this doesn't happen? Could we statically guarantee that somehow?
| ) -> Result<oneshot::Receiver<ResponseResult>, Box<EncodeError>> { | ||
| let msgid = self.msgid_counter.fetch_add(1, Ordering::SeqCst); | ||
|
|
||
| fn get_args<T: Serialize + fmt::Debug>( |
There was a problem hiding this comment.
Is this inlined for a specific reason? I'd prefer if it weren't, unless I'm missing something.
| @@ -119,10 +140,10 @@ where | |||
| Ok(receiver) | |||
| } | |||
|
|
|||
| pub async fn call( | |||
| pub async fn call<T: Serialize + fmt::Debug>( | |||
There was a problem hiding this comment.
Why are you using fmt::Debug instead of simply Debug here?
|
|
||
| /// Pack the given arguments into a `Vec<Value>`, suitable for using it for a | ||
| /// [`call`](crate::neovim::Neovim::call) to neovim. | ||
| #[macro_export] | ||
| macro_rules! call_args { | ||
| () => (Vec::new()); | ||
| () => { |
There was a problem hiding this comment.
What's this one for? If we need an empty Vec<Value> I think type inference should usually figure that out when just using vec![], right?
| self | ||
| .call("nvim_call_function", call_args![fname, args]) | ||
| .call("nvim_call_function", Args(fname, args)) |
There was a problem hiding this comment.
Are there any blanket impl's we could take advantage of here? Maybe we can just use (fname, args)? Otoh, this might just be clearer to read.
| @@ -265,13 +365,13 @@ impl IntoVal<Value> for Vec<(Value, Value)> { | |||
| } | |||
| } | |||
|
|
|||
| #[cfg(all(test, feature = "use_tokio"))] | |||
| // #[cfg(all(test, feature = "use_tokio"))] | |||
There was a problem hiding this comment.
What's up with this? One can't use #[tokio::test] if the feature isn't enabled. We're not introducing a general dependency on tokio here, do we?
|
|
||
| /// Syncronously decode the content of a reader into an rpc message. Tries to | ||
| /// give detailed errors if something went wrong. | ||
| fn decode_buffer_other<R: Read>( |
There was a problem hiding this comment.
Is this just kept as a comparison? Doesn't serve a purpose in the long run, right?
| @@ -66,12 +168,24 @@ pub async fn decode<R: AsyncRead + Send + Unpin + 'static>( | |||
| *rest = rest.split_off(pos as usize); // TODO: more efficiency | |||
| return Ok(msg); | |||
| } | |||
|
|
|||
| Err(DecodeError::BufferError(e)) | |||
There was a problem hiding this comment.
Is this variant still needed?
|
Maybe it would be far simpler to abstract |
|
That might just be feasible as well :) |
So I tried to manually do it then realized the code is generated, in generated code, I have gotten away with |
|
Not all is generated, have a look at https://github.com/KillTheMule/nvim-rs/blob/master/src/neovim_api_manual.rs. If we figure out something good, changing the generating script can follow. |
|
This branch has conflicts that must be resolved |
In your thoughts that you posted I saw that you wanted a strongly typed api and to hide
Valuefrom the user. I have implemented that usingrmp_serdeand thewith_serdefeature flag ofrmpv. This allowssend_msgto send anything with args that implementSerializeopening up possibilities for a strongly typed api because using structs asargswill also work. This waycallandcall_functioncan both take a wide range of values also. The base of this pr is implementingSerializeandDeserializeforRpcMessage. Before you were manually serializing and deserializing intoRpcMessage. Instead I have implemented the serde traits which is much easier and fits into the serde data model. Eventually I think that we don't needValueat all and can just rely onserde. This pr can open up very cool possibilities like having custom structs for common things likeCompleteItemsandQfListItems.Licensing: The code contributed to nvim-rs is licensed under the MIT or
Apache license as given in the project root directory.