Omit empty params in v2 request object #46
Conversation
lib/src/lib.rs
Outdated
| @@ -166,7 +166,8 @@ pub struct Request { | |||
| pub id: Id, | |||
| pub jsonrpc: Version, | |||
| pub method: String, | |||
| pub params: Params, | |||
| #[serde(skip_serializing_if = "Option::is_none")] | |||
There was a problem hiding this comment.
Can we get the same result if we slap a similar annotation onto the inner variants of Params?
That would allow us to not break the public API and release this as a patch.
If we have to use an Option, I would prefer if we:
- Initialize it to
None - Set it to
Someon the first call topush - Make
serializean immutable getter again
There was a problem hiding this comment.
Actually, we should only initialize Params to None for the v1 request. It is only for v2 where it can be omitted.
There was a problem hiding this comment.
So we can't use serde skip_if on enums with variants. In order to not break the pub on the struct, I have implemented Serializer to detect if map is empty.
There was a problem hiding this comment.
I have also updated change log and bumped version (since no public changes).
CHANGELOG.md
Outdated
| @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
|
|
|||
| ## [Unreleased] | |||
|
|
|||
| ## [0.7.1] - 2021-08-20 | |||
There was a problem hiding this comment.
Setting the version and date happens automatically as part of the release automation, so this line needs to be removed again sorry. A bit too ambitious :)
| ## [0.7.1] - 2021-08-20 |
lib/Cargo.toml
Outdated
| version = "0.7.0" | ||
| authors = [ "Thomas Eizinger <thomas@eizinger.io>" ] | ||
| categories = [ "web-programming::http-client", "api-bindings", "network-programming" ] | ||
| authors = ["Thomas Eizinger <thomas@eizinger.io>"] |
There was a problem hiding this comment.
What happened here? Can you please undo these formatting changes?
lib/src/lib.rs
Outdated
| where | ||
| S: Serializer, | ||
| { | ||
| let mut s = s.serialize_struct("request", 4)?; |
There was a problem hiding this comment.
| let mut s = s.serialize_struct("request", 4)?; | |
| let mut s = s.serialize_struct("Request", 4)?; |
There was a problem hiding this comment.
Also, the 4 isn't always correct, is it? I am not sure what implications it has but I think we need to make this dynamic based on whether we will be serializing params or not.
There was a problem hiding this comment.
Thank you! That looks good! :)
I am sorry about the CI test failure, I've fixed the test here: #48
Can you rebase onto latest master please?
…onrpc-client into cb/omit-empty-params-v2
|
@thomaseizinger think this is ready now? |
Yes. It does seem that you merged instead of rebased so the diff includes my commits as well but that is not so much of a big deal :) |
This pull request removes empty params from v2 request objects if they are empty.
Resolves #45