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

Proto3 Field Presence #49

Closed
sneako opened this issue Apr 14, 2021 · 3 comments · Fixed by #50
Closed

Proto3 Field Presence #49

sneako opened this issue Apr 14, 2021 · 3 comments · Fixed by #50
Assignees

Comments

@sneako
Copy link
Contributor

sneako commented Apr 14, 2021

First off, thanks for the great library! In my initial testing, it offers the best performance out of our options in the BEAM ecosystem, and I appreciate the property and mutation tests!

It appears that they have added field presence to proto3, so now it is possible again (as it was in proto2) to distinguish between a value that was not set at all and a value that was set to the default value.

See:

https://github.com/protocolbuffers/protobuf/blob/master/docs/field_presence.md
https://github.com/protocolbuffers/protobuf/blob/master/docs/implementing_proto3_presence.md
protocolbuffers/protobuf#1606

Would you be interested in either adding this feature to Protox or accepting a PR?

@ahamez
Copy link
Owner

ahamez commented Apr 14, 2021

Thank you, it's always nice to know that this library can be of some help 🙂!

I've been thinking about implementing this for a while, even though I didn't write anything yet (I don't think it's too much work , if I understood correctly, it just a matter of mapping optional to a synthetic oneof). So yes, I would be very interested in having this feature!

However, I don't know when I will be able to work seriously on this topic. So, if you think you can come up with a PR, I would gladly accept it! Otherwise, I'll do my best to add this feature, but I can't make any promise on the schedule.

@sneako
Copy link
Contributor Author

sneako commented Apr 14, 2021

Great! I took a look around codebase and I agree it doesn't seem like it should be too hard to implement. I'll take stab at it 😄

@ahamez
Copy link
Owner

ahamez commented Apr 14, 2021

Thank you! Don't hesitate if you have any question!

sneako added a commit to sneako/protox that referenced this issue Apr 15, 2021
sneako added a commit to sneako/protox that referenced this issue Apr 15, 2021
sneako added a commit to sneako/protox that referenced this issue Apr 15, 2021
sneako added a commit to sneako/protox that referenced this issue Apr 16, 2021
ahamez pushed a commit that referenced this issue Apr 20, 2021
@ahamez ahamez self-assigned this Sep 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants