Skip to content

Implement Deserialize and Serialize for plist::Dictionary and plist::Value#70

Merged
ebarnard merged 8 commits into
ebarnard:masterfrom
MLKrisJohnson:dictionary-serialization
Oct 25, 2021
Merged

Implement Deserialize and Serialize for plist::Dictionary and plist::Value#70
ebarnard merged 8 commits into
ebarnard:masterfrom
MLKrisJohnson:dictionary-serialization

Conversation

@MLKrisJohnson

@MLKrisJohnson MLKrisJohnson commented Aug 25, 2021

Copy link
Copy Markdown
Contributor

Implements serialization and deserialization of the Dictionary and Value types.

This fixes the nested-dictionary deserialization issue discussed in #54, and also allows plist files to be read and written as general Dictionary structs rather than as custom structs.

All the existing tests passed, and I don't think I've made any changes that would affect any existing code that uses this crate.

There are two limitations to this implementation:

  • Date elements in a plist are deserialized as String elements in the dictionary.
  • Uid elements are deserialized as Integer elements in the dictionary.

I tried to figure out how to fix these issues, but gave up. I think plist::value::Value::deserialize() or de:Deserializer::deserialize_any() need a change to properly handle these two newstruct types. However, even with these limitations, this code is useful. Maybe someone else can figure out how to complete the implementation for Date and Uid

@ebarnard

Copy link
Copy Markdown
Owner

Hi, thanks for the PR. Sorry, I completely forgot about it. I'll have a look at it in the next couple of days.

@gibfahn

gibfahn commented Oct 19, 2021

Copy link
Copy Markdown

Tried this out and it's working for me in a local project, @ebarnard if you get time to take a look it would be great to get this merged.

@ebarnard

Copy link
Copy Markdown
Owner

Looks good and I'm happy to merge this. I'd like to get Date and Uid round-tripping properly when a Value is deserialised from a plist file before releasing it though, and it might take me a while to get round to it myself. If you want to give it a shot, some guidance is below.

Serialisation is already working thanks to:

serializer.serialize_newtype_struct(DATE_NEWTYPE_STRUCT_NAME, &date_str)

Deserialisation just needs the visitor to implement visit_newtype_struct and switch on the newtype struct name, as is done in the serialiser:

DATE_NEWTYPE_STRUCT_NAME => value.serialize(DateSerializer { ser: &mut *self }),

@ebarnard

Copy link
Copy Markdown
Owner

I'm not sure why tests aren't being run but a push usually fixes that.

@MLKrisJohnson

Copy link
Copy Markdown
Contributor Author

I'll give this another try, with your hint to implement visit_newtype_struct

@MLKrisJohnson

MLKrisJohnson commented Oct 21, 2021

Copy link
Copy Markdown
Contributor Author

This is probably very simple, but I'm not familiar with the internals of serde and I get confused when I try to understand the relationship between visitors and deserializers. It seems the deserializer calls the visitor which calls the deserializer which calls the visitor which calls the deserializer....

If I understand your suggestion, I should add a method like this to the ValueVisitor implementation at https://github.com/MLKrisJohnson/rust-plist/blob/67caaa83110afa51d84f4d927ff03d9de72f392e/src/value.rs#L370

            fn visit_newtype_struct<D>(self, deserializer: D) -> Result<Value, D::Error>
            where
                D: Deserializer<'de>,
            {
                todo!();
            }

Then I should "switch on the newtype name", but this method doesn't have access to the newtype name. The plist::Deserializer type does have a deserialize_newtype_struct() method that would have access to the name, but that method currently calls visit_newtype_struct:

https://github.com/MLKrisJohnson/rust-plist/blob/67caaa83110afa51d84f4d927ff03d9de72f392e/src/de.rs#L182

However, it's not clear to me whether I should

  • change deserialize_newtype_struct so that it switches on the name, calls self.events.next() to read a string for Date or a u64 for Uid and then construct that kind of value. (This seems straightforward, but every deserialize_newtype_struct example I find says it should just call visit_newtype_struct so it feels like I'm missing something.)
  • change deserialize_any (at https://github.com/MLKrisJohnson/rust-plist/blob/67caaa83110afa51d84f4d927ff03d9de72f392e/src/de.rs#L91) so that it calls visitor.visit_newtype_struct for the Event::Date and Event::Uid cases, providing either self or some kind of type-specific deserializer.
  • somehow call DateNewtypeVisitor::visit_newtype_struct and UidNewtypeVisitor::visit_newtype_struct someplace
  • somehow call Date::deserialize() or Uid::deserialize() someplace

Can someone please point me in the right direction? Thanks!

@ebarnard

Copy link
Copy Markdown
Owner

I thought there was a way for a Deserializer to hint to a Deserialize/Visitor the name of an expected newtype struct but you're right, there isn't, it's the other way round.

I can think of a few other ways of doing this, but none are particularly nice so happy to merge as is, although you might have to wait a little bit for a release.

@MLKrisJohnson

MLKrisJohnson commented Oct 22, 2021

Copy link
Copy Markdown
Contributor Author

I'd like to get this right, but I don't really have a strategy at the moment. I'd say go ahead and merge it if you're happy with it.

Do you need me to make another push or something to get the tests to run? (It looks like tests are failing because they consider serde to be undeclared, but that doesn't make sense. Tests run fine on my machine.)

@ebarnard

ebarnard commented Oct 22, 2021

Copy link
Copy Markdown
Owner

Serde is only enabled with #[cfg(feature = “serde”)] which is why the tests are failing. I suggest putting all the serde stuff in a module like is done in

pub mod serde_impls {

@MLKrisJohnson

Copy link
Copy Markdown
Contributor Author

I've moved code around so that the tests run. I think this is ready for merging.

@ebarnard

Copy link
Copy Markdown
Owner

Looks great, thanks for doing all this work

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.

3 participants