Using generated code for manifest serializing/deserializing instead of custom parsers #276
Conversation
| } | ||
| } | ||
| #[test] | ||
| fn manifest() { |
There was a problem hiding this comment.
Could you please add tests that:
- Verify that the default edition is
CANARYduring serialization - Serialization fails when the edition doesnt match the format specified?
Also (nit) could you group them in a test mod?
| #[serde(untagged)] | ||
| enum RawManifest { | ||
| Canary { | ||
| edition: Edition, |
There was a problem hiding this comment.
This will most likely break stuff.
The old deserialization code mapped multiple different versions to the RawManifest::Canary format. In the future this is great to maintain backwards compat: Ie. you can introduce a new format of the RawManifest (e.g. V1) and determine into which one to deserialize by looking at the edition field first.
This MR lost that capability as far as i can tell: You deserialize everything that has an edition into Canary and everything without into Unknown (which is indeed the current behaviour, but rules out easy adoption of breaking manifest changes). If you now add the third variant to your code, you are not inspecting the edition format pre decoding.
One option i could see to recover this (if you feel strongly about not having manual deserialization in place) is to add newtypes around Edition which represents the subsets supported by one RawManifest variant.
mara-schulke
left a comment
mara-schulke
left a comment
There was a problem hiding this comment.
Happy to change / get rid of the manual serialization / deserialization code! Thanks for raising this. My main concerns evolve around the constraints wrt. edition management! If we can bring back the "This edition must serialize into this raw format" then im happy to roll with this!
2 participants
This is just simple code cleanup, and will allow to add more formats to the manifest.