Skip to content
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

Make fields optional for non-primitive protobuf fields #92

Merged
merged 2 commits into from
May 28, 2019

Conversation

noelmarkham
Copy link
Contributor

Added a new field, TOptionalNamedType for higherkindness/mu-scala#612 - Protobuf requiring optional fields for non-primitive types.

Would appreciate feedback on this; I feel this can't be everything I need to do for this to be complete.

Copy link
Member

@pepegar pepegar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @noelmarkham , thanks for your contribution!

I would avoid adding new cases to ASTs, which make it difficult to make transformations (and will make it harder to migrate to the UAST later, you can see what UAST is here #61)

can you try representing the same thing using a TOption(TNamedType(name).embed)?

@noelmarkham noelmarkham changed the title Introduce a new TOptionalNamedType for non-primitive protobuf fields Make fields optional for non-primitive protobuf fields May 24, 2019
Copy link
Member

@pepegar pepegar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks @noelmarkham !

@eli-jordan
Copy link

I think you will need to have separate handling for proto3 vs proto2 syntax. In proto2 fields are optional by default but can be marked as required. In proto3 all fields are always optional.

@noelmarkham noelmarkham merged commit 830a044 into master May 28, 2019
@noelmarkham noelmarkham deleted the feature/optional-protobuf-message-params branch May 28, 2019 08:38
@pepegar
Copy link
Member

pepegar commented May 28, 2019

@eli-jordan Yep, we're planning to have different printers in the future for proto2 and proto3. Thanks for your comment!

@TannerYoung
Copy link

Is there a follow up bug explicitly for proto2 vs proto3 support for primitive fields?

The appeal of proto2 is that you can tell whether a field has been explicitly set. For example
with strings in proto3 it is impossible to differentiate between not set and the empty string, but this is possible with proto2.

It feels a bit odd that this support only extends to non-primitive fields.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants