Correctly support proto3 optional fields in commonjs+dts .d.ts output #1184
Conversation
|
|
|
I think this is correct, I'm not entirely sure how to build or test it. Guidance would be greatly appreciated. |
|
I've managed to build (via bazel) and test this locally against some proto files I have with optional fields and it correctly outputs hasXxx and clearXxx methods for those fields in the .d.ts files. |
There was a problem hiding this comment.
@mattnathan Hi Matt! Thanks so much for working on this fix (and apologize for the delay)!! The change (logic) looks great to me! 😃
Lemme look a little bit into what are some good ways to unit test it. (If not i'd say just merge it anyways since it's probably low risk :D)
@stanley-cheung Lemme know if you have some ideas on how you would test this change too! Thanks! :)
| if ((field->type() == FieldDescriptor::TYPE_MESSAGE || field->has_optional_keyword()) && | ||
| !field->is_repeated() && !field->is_map()) { |
There was a problem hiding this comment.
nit: i'd probably prefer to maybe rewrite the logic as follows:
(field->has_optional_keyword() ||
(field->type() == FieldDescriptor::TYPE_MESSAGE &&
!field->is_repeated() &&
!field->is_map())
Probably doesn't make a huge difference, but it seems to make a little bit more sense to me.. :P
WDYT? :)
There was a problem hiding this comment.
I'm not sure.
The semantics of the two conditions is only the same when you factor in the proto3 language spec disallowing optional on map or repeated fields.
I read it as when the field is a message or optional, but not when repeated or a map as opposed to when the field is optional (which implies it's not repeated or a map), or it's a message but not repeated or a map.
I'll leave the final decision up to you though :)
There was a problem hiding this comment.
The semantics of the two conditions is only the same when you factor in the proto3 language spec disallowing optional on map or repeated fields.
Yeah i think that was my exactly concern.. -- that optional is mutually exclusive with repeated and map so i don't think they should be in a AND relationship.. :)
I read it as when the field is a message or optional, but not when repeated or a map as opposed to when the field is optional (which implies it's not repeated or a map), or it's a message but not repeated or a map.
Yeah right.. although i felt that's a little long-winded.. (and i had some challenge understanding that too) 😅
Where i was thinking as: when the field is optional, or a message that is not repeated or a map.
I feel the latter is simpler and easier to reason. :)
If you are ok both ways, do you mind making the change? Thanks! :)
There was a problem hiding this comment.
The change has been made
Not sure what the process is around resolving conversations, so I'll leave this thread here
There was a problem hiding this comment.
Thanks a lot for the change! I guess it doesn't hurt to just leave them open here :D
| printer->Print( | ||
| vars, "get$js_oneof_name$Case(): $class_name$.$js_oneof_name$Case;\n"); | ||
| printer->Print("\n"); | ||
| if (!oneof->is_synthetic()) { |
There was a problem hiding this comment.
Btw nice job finding the is_synthetic() API!😃
There was a problem hiding this comment.
Thanks, you forget what it's like to find docs via Google instead of using your IDE 😂
|
Sorry for any delays in responses, UK based over here |
Np at all! That's almost no delay at all (compared it taking 16 days for me to review 😂).. Thanks again for the contribution and being patient with the review! :) (I will cut a release very soon (maybe today or latest next week) which contains this fix. Thanks! :)) |
Fixes #1072