tokio-rs / valuable

MIT License
185 stars 19 forks source link

serde: serialize `dyn Error` `Value`s using `collect_str` #89

Closed hawkw closed 2 years ago

hawkw commented 2 years ago

Currently, valuable-serde handles Value::Error variants incorrectly. When valuable-serde encounters a Value::Error, it returns the error immediately as a serialization error: https://github.com/tokio-rs/valuable/blob/1fc2ad50fcae7fc0da81dc40a385c235636ba525/valuable-serde/src/lib.rs#L267

This is not correct. If a serde serializer returns an error, this means something has gone wrong while serializing a value. Returning an error makes the serialization fail. However, this is not what valuable's Value::Error means. Value::Error represents that we are recording a value which happens to be an Error, not that something went wrong while recording the value. This means that valuable-serde will currently return an Error (indicating that serialization failed) any time it attempts to record an Error value, and serialization will fail.

This commit changes valuable-serde to record the error using its Display implementation, using serde's collect_str method. Now, Errors will be serialized by recording their messages as a string, rather than causing the serialization to fail.

Using collect_str allows the serializer to write the display representation to its own internal buffer, rather than having to format the error to a temporary String beforehand.

Fixes #88

taiki-e commented 2 years ago

lgtm.

As for the CI (codegen) failure, I implemented a way to automatically fix that (https://github.com/rust-lang/futures-rs/pull/2562), but forgot to port it to this repository. I will open a PR later.

hawkw commented 2 years ago

@taiki-e great, happy to rebase this once CI is sorted out!

taiki-e commented 2 years ago

@hawkw: Filed #90 to fix CI issue.