PookieBuns commented on PR #519: URL: https://github.com/apache/avro-rs/pull/519#issuecomment-4139325260
Hi @Kriskras99, MR should be as minimal as possible right now to provide the functionality to address #517. However, I think there are some parts of code that could be DRYed up and refactored to be more understandable. More specifically, I think separating out fully `UnionSerializer`, `OptionUnionSerializer`, `OptionSerializer` and `Serializer` for schema aware serialization (and deserialization) would help prevent the current implementation from having (zig) or (zag) spread across the implementations and dealing with edge cases (For example, in deserialize, if we zag first when deserializing option, all code following that now needs a check to prevent zagging again). But that's I think out of scope for now, and if you want me to, I can separately work on that after this. Please lmk if you have any questions! Oh also please let me know if I should keep or get rid of the nullable union tests(or those that are intended to fail). I find them quite valuable for asserting correctness. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
