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

Major refactor: Rename methods, variant index, struct fields, deserializing structs from seq, pulling out json, and cleanup #114

Merged
merged 14 commits into from Jul 23, 2015

Conversation

5 participants
@erickt
Member

erickt commented Jul 22, 2015

This is a series of breaking changes in order to better support formats like bincode:

  • Bump version number to 0.5.0
  • Renames methods like visit_named_{map,seq} to visit_{struct,tuple,tuple_struct}. This better reflects how their used.
  • Passes the struct fields names to the deserializer.
  • Allow structs to be deserialized from sequences.
  • Added constructor to tuple deserializer visitor
  • Moved json into it's own crate
  • Other minor cleanup

Closes #110

cc @pcwalton

@erickt

This comment has been minimized.

Show comment
Hide comment
@erickt

erickt Jul 22, 2015

Member

Some open questions:

  • Since I'm queueing some some breaking changes, what other breaking changes are there that should be done in 0.5.0?
  • Argument lists are starting to get a bit noisy. Visiting enums now ends up passing in 4 arguments: the variant index, the enum name, the variant name, then the next visitor. Deserializing structs now passes in the struct field arguments. Is it worth while trying to either capture these arguments in some context struct, or do some fancy trait trickery to get these arguments exposed? I actually implemented passing this info in a traits, but I ended up having to generate 5 impls, which seemed excessive.
  • Is it worth passing in the field index to the serializer? It can already track the index on it's side, but it could be slightly more consistent with enum variants passing in the index.
Member

erickt commented Jul 22, 2015

Some open questions:

  • Since I'm queueing some some breaking changes, what other breaking changes are there that should be done in 0.5.0?
  • Argument lists are starting to get a bit noisy. Visiting enums now ends up passing in 4 arguments: the variant index, the enum name, the variant name, then the next visitor. Deserializing structs now passes in the struct field arguments. Is it worth while trying to either capture these arguments in some context struct, or do some fancy trait trickery to get these arguments exposed? I actually implemented passing this info in a traits, but I ended up having to generate 5 impls, which seemed excessive.
  • Is it worth passing in the field index to the serializer? It can already track the index on it's side, but it could be slightly more consistent with enum variants passing in the index.
@pcwalton

This comment has been minimized.

Show comment
Hide comment
@pcwalton

pcwalton Jul 22, 2015

Contributor

I don't have much of an opinion on any of those questions, but this PR seems great!

Contributor

pcwalton commented Jul 22, 2015

I don't have much of an opinion on any of those questions, but this PR seems great!

@@ -92,6 +92,9 @@
//! }

This comment has been minimized.

@tomprogrammer

tomprogrammer Jul 22, 2015

Contributor

The doctest above refers to serde::json instead of serde_json.

@tomprogrammer

tomprogrammer Jul 22, 2015

Contributor

The doctest above refers to serde::json instead of serde_json.

This comment has been minimized.

@erickt

erickt Jul 22, 2015

Member

Fixed, thanks!

@erickt

erickt Jul 22, 2015

Member

Fixed, thanks!

@tomprogrammer

This comment has been minimized.

Show comment
Hide comment
@tomprogrammer

tomprogrammer Jul 22, 2015

Maybe the commit message (relies on ordering of the sequence) should be copied into a comment here to clarify that this method is stricter than the json specification requires.

UPDATE: Nevermind, sequences are ordered in json too, just objects are unordered key-value-pairs. So this commit is good as it is.

tomprogrammer commented on serde_codegen/src/de.rs in dbe2bea Jul 22, 2015

Maybe the commit message (relies on ordering of the sequence) should be copied into a comment here to clarify that this method is stricter than the json specification requires.

UPDATE: Nevermind, sequences are ordered in json too, just objects are unordered key-value-pairs. So this commit is good as it is.

@tomprogrammer

This comment has been minimized.

Show comment
Hide comment
@tomprogrammer

tomprogrammer Jul 22, 2015

Contributor

Great PR, finally serde is a generic serialization framework with json split out in a own repo.

Contributor

tomprogrammer commented Jul 22, 2015

Great PR, finally serde is a generic serialization framework with json split out in a own repo.

@oli-obk oli-obk referenced this pull request Jul 23, 2015

Closed

port to serde 0.5.0 #5

@oli-obk

This comment has been minimized.

Show comment
Hide comment
@oli-obk

oli-obk Jul 23, 2015

Member

you got #115 mangled up in this PR, otherwise lgtm

Member

oli-obk commented Jul 23, 2015

you got #115 mangled up in this PR, otherwise lgtm

@erickt erickt merged commit 57753c9 into serde-rs:master Jul 23, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hugoduncan

This comment has been minimized.

Show comment
Hide comment
@hugoduncan

hugoduncan Jul 23, 2015

Contributor

other breaking changes are there that should be done in 0.5.0?

I can't think of any. Converting nested enum/structs to a flat list of "dotted" keys currently requires some work e.g for AWS requests, but I'm not sure if that points to a need for some sort of cumulative "context" to be passed to each visitor.

Argument lists are starting to get a bit noisy

The argument lists don't seem too bad to me, and I don't think they warrant the added complexity of trait trickery or defining a composite type to pass.

Is it worth passing in the field index to the serializer? It can already track the index on it's side, but it could be slightly more consistent with enum variants passing in the index.

What is the use case for passing the enum variant index, and does that use case exist for the field index? or for the sequence element index?

Contributor

hugoduncan commented Jul 23, 2015

other breaking changes are there that should be done in 0.5.0?

I can't think of any. Converting nested enum/structs to a flat list of "dotted" keys currently requires some work e.g for AWS requests, but I'm not sure if that points to a need for some sort of cumulative "context" to be passed to each visitor.

Argument lists are starting to get a bit noisy

The argument lists don't seem too bad to me, and I don't think they warrant the added complexity of trait trickery or defining a composite type to pass.

Is it worth passing in the field index to the serializer? It can already track the index on it's side, but it could be slightly more consistent with enum variants passing in the index.

What is the use case for passing the enum variant index, and does that use case exist for the field index? or for the sequence element index?

@erickt

This comment has been minimized.

Show comment
Hide comment
@erickt

erickt Jul 23, 2015

Member

@hugoduncan: The enum variant index is necessary for a format like bincode, that uses it to more efficiently encode which variant a value is. For structs, it actually encodes them as a tuple, so the field position is implied in the tuple ordering. The only scenario I could think of where you'd want to encode the field index is if you're deserializing an out of order Map<u32, T> value, and you want to map it to a struct. This PR adds supports for that. I can't really think of another scenario, so I think we're probably good right now.

Member

erickt commented Jul 23, 2015

@hugoduncan: The enum variant index is necessary for a format like bincode, that uses it to more efficiently encode which variant a value is. For structs, it actually encodes them as a tuple, so the field position is implied in the tuple ordering. The only scenario I could think of where you'd want to encode the field index is if you're deserializing an out of order Map<u32, T> value, and you want to map it to a struct. This PR adds supports for that. I can't really think of another scenario, so I think we're probably good right now.

@erickt

This comment has been minimized.

Show comment
Hide comment
@erickt

erickt Jul 23, 2015

Member

@hugoduncan: On passing a context object around, I've thought about that and did end up experimenting with it back in the day. I ended up deciding that it was sufficient to store the state on the Serializer object. It felt like a cleaner api than having many formats having to thread a () object through the process. I'm always open to re-evaluating of course.

And that aws format looks to be quite screwy :) Amazon, json is a hierarchical file format, why not take advantage of it!

Member

erickt commented Jul 23, 2015

@hugoduncan: On passing a context object around, I've thought about that and did end up experimenting with it back in the day. I ended up deciding that it was sufficient to store the state on the Serializer object. It felt like a cleaner api than having many formats having to thread a () object through the process. I'm always open to re-evaluating of course.

And that aws format looks to be quite screwy :) Amazon, json is a hierarchical file format, why not take advantage of it!

@oli-obk

This comment has been minimized.

Show comment
Hide comment
@oli-obk

oli-obk Jul 23, 2015

Member

other breaking changes are there that should be done in 0.5.0?

how about passing length to the deserialization visit function for fixed size sequences like tuples? (and fixed size arrays?)

Member

oli-obk commented Jul 23, 2015

other breaking changes are there that should be done in 0.5.0?

how about passing length to the deserialization visit function for fixed size sequences like tuples? (and fixed size arrays?)

@erickt

This comment has been minimized.

Show comment
Hide comment
@erickt

erickt Jul 23, 2015

Member

@oli-obk: as in this method? https://github.com/serde-rs/serde/blob/master/serde/src/de/mod.rs#L223. How do you think it'd be used? Hm. Maybe for formats that prefix a sequence with a length could exit early if they don't match up with the destination type.

Member

erickt commented Jul 23, 2015

@oli-obk: as in this method? https://github.com/serde-rs/serde/blob/master/serde/src/de/mod.rs#L223. How do you think it'd be used? Hm. Maybe for formats that prefix a sequence with a length could exit early if they don't match up with the destination type.

@oli-obk

This comment has been minimized.

Show comment
Hide comment
@oli-obk

oli-obk Jul 23, 2015

Member

in xml it's necessary to be able to deserialize sequences of sequences, as a sequence is just loads of elements with the same tag. So a sequence of n tuples of m arity is n*m elements with the same tag. no way to deserialize without knowing the arity. of course Vec<Vec> still doesn't work

Member

oli-obk commented Jul 23, 2015

in xml it's necessary to be able to deserialize sequences of sequences, as a sequence is just loads of elements with the same tag. So a sequence of n tuples of m arity is n*m elements with the same tag. no way to deserialize without knowing the arity. of course Vec<Vec> still doesn't work

rubdos pushed a commit to rubdos/serde that referenced this pull request Jun 20, 2017

Auto merge of #114 - achanda:is_nan, r=hauleth
Implement is_nan for the complex type

Also run the associated test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment