Solve compiler error when serde is not a dependency of user project (#481)#498
Solve compiler error when serde is not a dependency of user project (#481)#498tomusdrw merged 1 commit intoparitytech:masterfrom suguruwataru:master
Conversation
|
It looks like @simeir hasn't signed our Contributor License Agreement, yet.
You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io Once you've signed, please reply to this thread with Many thanks, Parity Technologies CLA Bot |
tomusdrw
left a comment
There was a problem hiding this comment.
This actually happens to be even worse than previously, cause previously we were allowing to rename serde in the Cargo.toml (see crate_name("serde")) now it has to be named serde and still must be imported in the top level Cargo.toml.
We should rather re-export serde from jsonrpc-core or derive and use that instead of relying on serde directly.
I actually tested using another crate and this does not seem to be case. I can check again, though. I see what's going on now. I re-exported the By the way, can you tell me about the CLA thing? |
|
[clabot:check] |
|
It looks like @simeir signed our Contributor License Agreement. 👍 Many thanks, Parity Technologies CLA Bot |
tomusdrw
left a comment
There was a problem hiding this comment.
Great! Looks good now, just one tiny improvement to allow renaming jsonrpc-core upon importing.
derive/src/to_delegate.rs
Outdated
| if client { | ||
| if visitor.server_to_client_type_params.contains(&ty.ident) { | ||
| bounds.push(parse_quote!(_serde::de::DeserializeOwned)) | ||
| bounds.push(parse_quote!(jsonrpc_core::serde::de::DeserializeOwned)) |
There was a problem hiding this comment.
please use #core_name instead of jsonrpc_core. This allows using:
rpc = { package = "jsonrpc-core", ... }
inside the Cargo.toml - so the crate is renamed, and in such case your jsonrpc_core import would not work.
Note that the code we generate here is running in the context of the user's crate, so we don't want to enforce them to import jsonrpc-core in a specific way.
| bounds.push(parse_quote!(jsonrpc_core::serde::de::DeserializeOwned)) | |
| bounds.push(parse_quote!(#core_name::serde::de::DeserializeOwned)) |
There was a problem hiding this comment.
I think it might be better to have _jsonrpc_core instead of #core_name, as the name is already imported as _jsonrpc_core and all other parts of the code are using _jsonrpc_core to refer to the crate.
derive/src/to_delegate.rs
Outdated
| } | ||
| if visitor.client_to_server_type_params.contains(&ty.ident) { | ||
| bounds.push(parse_quote!(_serde::Serialize)) | ||
| bounds.push(parse_quote!(jsonrpc_core::serde::Serialize)) |
There was a problem hiding this comment.
| bounds.push(parse_quote!(jsonrpc_core::serde::Serialize)) | |
| bounds.push(parse_quote!(#core_name::serde::Serialize)) |
derive/src/to_delegate.rs
Outdated
| } else { | ||
| if visitor.server_to_client_type_params.contains(&ty.ident) { | ||
| bounds.push(parse_quote!(_serde::Serialize)) | ||
| bounds.push(parse_quote!(jsonrpc_core::serde::Serialize)) |
There was a problem hiding this comment.
| bounds.push(parse_quote!(jsonrpc_core::serde::Serialize)) | |
| bounds.push(parse_quote!(#core_name::serde::Serialize)) |
derive/src/to_delegate.rs
Outdated
| } | ||
| if visitor.client_to_server_type_params.contains(&ty.ident) { | ||
| bounds.push(parse_quote!(_serde::de::DeserializeOwned)) | ||
| bounds.push(parse_quote!(jsonrpc_core::serde::de::DeserializeOwned)) |
There was a problem hiding this comment.
| bounds.push(parse_quote!(jsonrpc_core::serde::de::DeserializeOwned)) | |
| bounds.push(parse_quote!(#core_name::serde::de::DeserializeOwned)) |
|
Re: CLA, we require signing it off mainly to be able to change license in the future (if needed) without getting a consent from all contributors. Not a lawyer though (and this is not an advice), so please make sure you read it and understand all the aspects. |
Got it. Thanks! |
…481) Use `serde` directly instead of ask for user of the library to add it as a dependency. Tests are modified since this is a ui change. The doc comment gives an example using `#[rpc(server)]` instead of original `#[rpc]`, telling users that this attribute has an option to be used to configure it.
Use
serdedirectly instead of ask for user of the library to add it as adependency.
Tests are modified since this is a ui change.
The doc comment gives an example using
#[rpc(server)]instead of original#[rpc], telling users that this attribute has an option to be used toconfigure it.