-
Notifications
You must be signed in to change notification settings - Fork 20
cm: implement JSON [un]marshaling for Option types#301
cm: implement JSON [un]marshaling for Option types #301brooksmtownsend wants to merge 4 commits into
Conversation
Signed-off-by: Brooks Townsend <brooksmtownsend@gmail.com>
Signed-off-by: Brooks Townsend <brooksmtownsend@gmail.com>
@ydnar
ydnar
left a comment
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.
Thanks!
This seems roughly right, but how would option<option> serialize and deserialize symmetrically?
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.
Methods should be on option[T] so named option types inherit the methods.
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 there an example I can follow for a named option type? I assumed that if external implementations named a type it would be for Option[T], perhaps I should implement the un/marshaling for both?
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.
// This type would not have the JSON methods. type OptionalI32 cm.Option[int32]
This is why the methods are implemented on option, which is embedded in Option, so methods are preserved.
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.
@ydnar added a few more test cases that show that this works for named and nested options 🫡
I did find that I could remove the Option[T].MarshalJSON implementation and still have tests work, but removing Option[T].UnmarshalJSON did fail the cases that we have, so I left both in.
Signed-off-by: Brooks Townsend <brooksmtownsend@gmail.com>
Signed-off-by: Brooks Townsend <brooksmtownsend@gmail.com>
@ydnar
ydnar
left a comment
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.
- Can you convert the tests to table driven?
- Please remove the methods on Option[T]. They will not be used for named option types.
- Please use a serialization format that is symmetrical. "null" for none is not it.
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.
This is not necessary.
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.
This is not necessary.
This PR implements (naively) option marshaling/unmarshaling for Option types. This case is fairly straightforward, marshal to
nullif None and to the value if present.Opening this as a draft just to ensure that this is a change that's well received that follows contribution structure, open to changing anything here!
Addresses Option un/marshaling for #239