refactor: support proto2 optional #162
Conversation
| @@ -847,6 +851,11 @@ function generateEncode( | |||
| ) | |||
| .addStatement('%L', writeSnippet(`message.${fieldName}`)) | |||
| .endControlFlow(); | |||
| } else if (field.label === FieldDescriptorProto.Label.LABEL_OPTIONAL) { | |||
There was a problem hiding this comment.
where is proto3Optional handled in this if/elseif/.../else block? Should that not also be handled here?
| return ( | ||
| (options.useOptionals && isMessage(field) && !isRepeated(field)) || | ||
| field.proto3Optional || | ||
| field.label === FieldDescriptorProto.Label.LABEL_OPTIONAL |
There was a problem hiding this comment.
really seems like we could use an isOptionalField helper...
| @@ -377,7 +377,7 @@ export function toTypeName( | |||
| field: FieldDescriptorProto, | |||
| options: Options | |||
| ): TypeName { | |||
| let type = basicTypeName(typeMap, field, options, { keepValueType: false }); | |||
| let type = basicTypeName(typeMap, field, options, { keepValueType: true }); | |||
| (!isWithinOneOf(field) && | ||
| isMessage(field) && | ||
| !options.useOptionals && | ||
| field.label !== FieldDescriptorProto.Label.LABEL_REQUIRED) || |
|
Any chance we can get this merged? :) |
|
@elderapo unfortunately the PR is pretty out of date, and had several unanswered questions from the original review, plus an ask for a test, so as-is, chances of being merged are low. If you'd like to pick it up, rebase on |
|
@ssilve1989 any change to merge? :) |
|
@philikon any change to merge? :) |
|
Please merge this 🙏 |
stephenh
force-pushed
the
main
branch
4 times, most recently
from
cdf2835
to
7017d4c
Compare
|
A lot of work for proto2 optional fields was just done in #1007 that hopefully addresses this, so going to close this out. |
References #139