Hide Value from user and use serde instead#19
Hide Value from user and use serde instead#19oberblastmeister wants to merge 7 commits intoKillTheMule:masterfrom
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. |
| } | ||
|
|
||
| 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.
| } | ||
|
|
||
| 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?
| #[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.
| } | ||
|
|
||
| #[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?
| 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 |
4 participants
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.