Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deserialize chart #129

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

jasoncg
Copy link

@jasoncg jasoncg commented Nov 22, 2024

Adds Deserialize support to various data structures in order to support deserializing Chart, as well as a couple extremely basic test cases to test deserialization.

Most of this was straightforward, just adding Deserialize to the derive attribute. But the following have custom Serialize implementations, so I wrote custom Deserialize implementations to match:

  • datatype::Dataset
  • series::Series
  • series::ThemeRiverData
  • element::axis_line
  • element::BoundaryGap
  • element::FontFamily
  • element::Icon
  • element::MarkLineVariant
  • element::Padding
  • element::RawString
  • element::Symbol

The other change was some vectors have skip_serializing_if = "Vec::is_empty", which was causing problem with deserialization since there wasn't anything to initialize these vectors. For all of those I set #[serde(default)] so that they can deserialize with empty vectors.

@LukaOber
Copy link
Collaborator

A lot of effort went into this, thank you very much. Would you be able to create unit tests for each custom Deserializer?

@LukaOber LukaOber self-requested a review November 30, 2024 10:42
@LukaOber
Copy link
Collaborator

I took a detailed look at the code. I tried implementing some unit tests myself and I think that the implemented Deserialization of Dataset is wrong, I haven't really tested the others, so likely more. Unit testing these implementations is definitely needed.

Do you have the time to add the unit tests? If not, I will try to correct the wrong Deserialization and provide some tests. But I am not super familiar with Deserialization.

I used serde_assert for my testing and I think it is a good choice for the unit tests. Just a simple roundtrip should be good enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants