Optional oneof #235
Optional oneof #235
Conversation
Now oneofs there are also properly encoded as options
|
So, this would prompt a ocaml-protoc 4.0 release? :) Maybe we should create a branch 4.0 for that then, and merge this into it. |
| @@ -411,21 +411,6 @@ let compile_message ~(unsigned_tag : bool) (file_options : Pb_option.set) | |||
| } | |||
| in | |||
| [ type_ ] | |||
| | Tt.Message_oneof_field f :: [] -> | |||
There was a problem hiding this comment.
does this mean we don't have a way to represent "just a sum type" anymore? That would make the OCaml code quite a lot heavier I think. We could perhaps keep this as a sum type definition, and return a _ option of it?
There was a problem hiding this comment.
Hm, that branch was special-casing a message which only include a oneof field. Normally ocaml-protoc emits two types - one for the variant and one for the message itself, including a field for that variant. With special-casing, there was no record type for the message itself, but only the one for the variant, which had the name for the whole message. With ordinary flow there is still a type for the variant, it's just named with _oneof suffix.
Your point is that when messages are organized into oneof only to be referenced from other messages, we'll have additional record wrapping and you want to remove it and have message type be option of sum type?
To my taste I find this special-casing confusing, I was unable to find my message record and it took me some time to figure out that it only had single one-of and the variant itself is what I need. Protobuf is about extending messages, and the odds are high that message with only one oneof field will get additional fields along the way, type errors will be quire confusing in this case (for the record case, if we don't care for exact field set and bind rest of fields to _, adding a field to message might require no changes to the program to still compile).
That's what I'm saying, yeah :)
Sounds good! Maybe something else would creep in with breaking changes. |
|
Alright, this is merged into the new |
|
Thanks for looking into this! |
This PR introduces necessary changes to encode OneOf fields as options in OCaml types, following the discussion in #227.
@c-cube are you sure it's worth it? Changes in this PR are quite spread across the codebase, and dragging additional parameter across all call stacks to make this behavior opt-in sounds like a bit too much of effort.