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

Add an option not to wrap some values in Options when generating Scala values #40

Closed
eiennohito opened this issue Sep 8, 2015 · 8 comments

Comments

@eiennohito
Copy link
Contributor

Sometimes zero is a good default and Option is just an overhead and one more level of indirection.
Especially, with proto3 making everything optional this is useful in my opinion.

Quickly hacked implementation: eiennohito@a3d20d4

@thesamet
Copy link
Contributor

First, thanks for suggesting and implementing this. Given that this is the default behavior with proto3, I would like to avoid complicating the logic for proto2s. Having said that, I would reconsider if we see that a request for this keeps coming.

@eiennohito
Copy link
Contributor Author

It is a default behavior on Protocol buffers language level. Java basically uses default values for everything. Using Options for primitives is two levels of indirection in JVM (Option + boxed value) and uses much more memory than a simple primitive field.

I agree about #43, that proto3 should be used, however this issue is a blocker for me personally. Option[T] means that there could be a T or could not be a T here, but proto3 just does not give an option to specify that there always will be something here in the interface definition. Scala's Options IMHO give more syntax garbage when there is always a value or default one does the job than Java generated classes for example. Probably going to use my fork for the time being.

@thesamet
Copy link
Contributor

thesamet commented Jun 10, 2016

Possible solution: add field-level option that tells the generator not to wrap a value in an option. Something like:

option Message msg = 1 [(scalapb).no_box = true]

This would apply to any field in proto2, and for proto3 this is relevant only to messages since primitives are not boxed.

Do we need anything file level to catch all as well?

@ahjohannessen
Copy link
Contributor

I would be smart if one could define this on file level as well, thus not being required to put that on all fields.

@tjdett
Copy link

tjdett commented Nov 18, 2016

I'd love to see [(scalapb).no_box = true] so google.protobuf.Timestamp can be non-optional with proto3.

@gurinderu
Copy link

@thesamet Do you have any news about this task?

@Voltir
Copy link

Voltir commented Sep 9, 2017

+1 - considering this is an application level validation step, I don't think it violates the spirit of "required is forever"

@allquantor
Copy link

+1 on this.

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

No branches or pull requests

7 participants