proto gen: support optionals for proto syntax 3 #105
Conversation
There was a problem hiding this comment.
Looks good to me, @aadedamola.
A couple of minor comments from me.
WDYT, @utrack ?
integration/binding_with_optional_field/strings/strings.to_upper.go
Outdated
Show resolved
Hide resolved
| @@ -203,17 +203,30 @@ func addValueTyped(f *descriptor.Field) string { | |||
| if valueFormatter != "" { | |||
| return fmt.Sprintf(valueFormatter, getter) | |||
| } | |||
|
|
|||
| if f.GetProto3Optional() { | |||
There was a problem hiding this comment.
is it possible to have both value formatter and optional for the same field?
I guess not, but could you please check it?
There was a problem hiding this comment.
We can have both -- currently, the valueformatter is used for encoding bytes. Bytes evaluate to the byte[] scalar type on go which is nullable. So the current implementation works.
The question is do we want to create pointers for bytes[] on code generation instead? And if yes, why?
There was a problem hiding this comment.
Agree. I don't think we need a pointer to a slice
| return fmt.Sprintf(`fmt.Sprintf("%s", %s)`, valueVerb, getter) | ||
| } | ||
|
|
||
| if !isRepeated { | ||
| if isRepeated { |
There was a problem hiding this comment.
It is not possible to have both optional and repeated for the same field, right?
It does not make much sense to me, but is it possible from the protobuf perspective?
There was a problem hiding this comment.
To the best of my knowledge, repeated fields are inherently optional. If we try to put both optional and repeated on the same field, the parser will raise an error (did a quick experiment btw)
There was a problem hiding this comment.
Also, an answer on Stack Overflow -- https://stackoverflow.com/a/25637833
|
LGTM, thanks! :) |
Based on the issue raised, this PR adds support for optional fields in proto 3. It also contains a test case for an optional field.