-
Notifications
You must be signed in to change notification settings - Fork 37
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
Explicit array serialization in serde #52
Explicit array serialization in serde #52
Conversation
/// to_writer( | ||
/// &mut serialized, | ||
/// &Sheep { | ||
/// byte_data: vec![0x62, 0x69, 0x6E, 0x61, 0x72, 0x79, 0x20, 0x73, 0x68, 0x65, 0x65, 0x70], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a "real" example? If so, it'd be neat to mention where it comes from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nah, just had a bit of fun with those examples. it's "binary sheep" in ASCII ;)
"__hematite_nbt_i8_array__" | ||
| "__hematite_nbt_i32_array__" | ||
| "__hematite_nbt_i64_array__" => Compound::for_seq(self.outer, len as i32, true), | ||
_ => Err(Error::UnrepresentableType(stringify!(tuple_struct))), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this just be UnrepresentableType("tuple struct")
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I basically just 1:1 copied what the macro did before, but I can change that if that's more readable
Edit: so stringify! does resolve to either "tuple_struct" or "tuple struct" dunno whether it removes the underscore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay will do
Can you squash this and rebase against master? |
Never done either of them. Squashing is putting multiple commits into one and rebase is that it looks like it branches off the current master HEAD? |
f157196
to
f820d92
Compare
Alright, it's done |
Thanks for all your work! |
This PR aims to implement Byte/Int/Long array serialization support for serde.
Right now every
Vec<i8>
or similar sequence types get serialized to aList
ofByte
s instead of theByteArray
type. This PR implements a serdeserialize_with
function to tag fields of collection types to serialize intoByteArray
/IntArray
/LongArray
(calledi8_array
/i32_array
/i64_array
respectively).For example:
This will serialize to a compound containing a
ByteArray
instead of aList
ofByte
s.Current limitations
hematite-nbt
. When serializing with a different serializer this will try to serialize as aserde
tuple_struct
with the name__hematite_nbt_i8_array__
(ori32
/i64
respectively) and the array elements as fields of that tuple.serialize_with
on inner elements (ofOption
,Vec
, ...) and thus nested arrays are not trivially supported (see Using de/serialize_with inside of an Option, Map, Vec serde-rs/serde#723). A workaround for this as also currently used inhematite_nbt
tests is not that clean, but it can be made to work:More details and discussion can also be found in the PR of a previous, different approach on implicitely converting all NBT lists that can be represented as arrays: #51
Resolves #27
Resolves #32