issuu / ocaml-protoc-plugin

ocaml-protoc-plugin
https://issuu.github.io/ocaml-protoc-plugin/
Other
48 stars 19 forks source link

Make proto3 optional fields into option types #33

Closed andersfugmann closed 2 years ago

andersfugmann commented 2 years ago

This change changes proto3 optional fields to be option types instead of an polymorphic variant. Currently proto3 optional fields are mapped as a oneof _<field_name> { <field_type> <field_name> = 1}. This maps to a polymorphic variant:

type t = { _<field_name>: [ `not_set | `<field_name> of <field_type> ]; ... }

The change proposed is to make optional fields in the same way as proto2:

type t = { <field_name>: <field_type> option; ... } 
andersfugmann commented 2 years ago

Apparently the github ubuntu runners does not have a sufficiently recent version of protoc (>= 3.12 required). Ill look into that later.

andersfugmann commented 2 years ago

Even worse - The tests would not even complete on any system with protoc < 3.12. Maybe we should just remove the extra tests (or somehow leave them as optional).

andersfugmann commented 2 years ago

Ok. Here is a fix for this little test problem and older compilers. By detecting if protoc compiler supports the new flag (I assume the flag will still be supported for protoc >= 3.15, where the flag is enabled by default), the build either includes or excludes testing the proto3_optional test.

Its convoluted, but it works, and its the simplest way I could think of. One drawback though is that the expect test is not really what we want, but the test will fail if proto3 options are not handled correctly.

andersfugmann commented 2 years ago

Thanks for reviewing. I will merge once the tests are green.

andersfugmann commented 2 years ago

I can create a new release over the weekend. I see you already tagged a v4.3. Should I override this tag, or create a new release tag?

AndreasDahl commented 2 years ago

Feel free to overwrite the 4.3.0 tag. I was intending to release it as-is, but I think we should rather have your changes in first. This was my release PR https://github.com/ocaml/opam-repository/pull/22085

andersfugmann commented 1 year ago

https://github.com/ocaml/opam-repository/pull/22090