-
Notifications
You must be signed in to change notification settings - Fork 56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Please add optional back to the AST #229
Comments
I don't think putting back optional on its own suffices. The spec gives a new behavior to it, which we'd have to take into account in the code as well! |
Is there any update on this issue? Is there a plan to support the optional fields by proto3-suite? |
I have tried within the past 6 months and adding optional back to the AST would minimally require a complete rework of how the protobuf AST is set up in I should also note that in addition to reworking the AST it would also be necessary to add compilation procedures handling |
@atriantafyllos-da we also welcome PRs if you feel inclined to try tackling it. We're pretty low on bandwidth right now at $job so you can't expect a lot of work except for that which affects us directly. If you do want to try tackling this, I think taking some time to understand the codebase, what @riz0id has said, and writing a small proposal/design for how to add the feature would be a great way to start. A design to review would be a lot easier on other maintainers with experience in the codebase to collaborate over. |
It looks like
Optional
was removed in this PR: https://github.com/awakesecurity/proto3-suite/pull/165/filesThe reason given was that it wasn't supported in proto3, but it is in fact part of the spec, see: https://protobuf.dev/programming-guides/proto3/#specifying-field-rules
It was erroneously left out of early documentation per: protocolbuffers/protobuf#10463
Can that PR be reverted?
The text was updated successfully, but these errors were encountered: