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

Implement the new proto3 field presence feature #34 #200

Merged
merged 2 commits into from
Mar 29, 2022

Conversation

NiematojakTomasz
Copy link
Contributor

Fix #34

Copy link
Collaborator

@garyp garyp left a comment

Choose a reason for hiding this comment

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

Thanks again for taking this on! The changes mostly look good to me. I have one question in FileBuilder.kt about something I'm confused about. The other comments are minor code style fixes.

The only other suggestion I have is to add some unit tests to runtime/commonTest to check that a few invariants are maintained when an instance of Proto3PresenceMain is encoded to a byte array and then that byte array is decoded back into a Proto3PresenceMain instance. Something along these lines (where msg is the result of calling encodeToByteArray() and then decodeFromByteArray()):

  • Proto3PresenceMain(optionalInt = 0, requiredInt = 0) => assert that msg.optionalInt == 0 and proto.requiredInt == 0
  • Proto3PresenceMain() => assert that msg.optionalInt == null and msg.requiredInt == 0
  • Maybe similar tests on some of the other data types like messages, enums, and oneof fields?

@NiematojakTomasz NiematojakTomasz force-pushed the proto3Optional branch 5 times, most recently from a4e6e74 to dd65832 Compare January 20, 2022 22:00
@NiematojakTomasz
Copy link
Contributor Author

Thanks again for taking this on! The changes mostly look good to me. I have one question in FileBuilder.kt about something I'm confused about. The other comments are minor code style fixes.

The only other suggestion I have is to add some unit tests to runtime/commonTest to check that a few invariants are maintained when an instance of Proto3PresenceMain is encoded to a byte array and then that byte array is decoded back into a Proto3PresenceMain instance. Something along these lines (where msg is the result of calling encodeToByteArray() and then decodeFromByteArray()):

  • Proto3PresenceMain(optionalInt = 0, requiredInt = 0) => assert that msg.optionalInt == 0 and proto.requiredInt == 0
  • Proto3PresenceMain() => assert that msg.optionalInt == null and msg.requiredInt == 0
  • Maybe similar tests on some of the other data types like messages, enums, and oneof fields?

Doesn't tests based on kotlinJavaRoundtripTest implicitly cover it? Or is it a matter of testing it explicitly?

Copy link
Collaborator

@garyp garyp left a comment

Choose a reason for hiding this comment

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

@NiematojakTomasz I got a chance to come back to this PR finally and add the unit tests I was suggesting. Sorry for the long delay and thank you again for contributing support for this feature!

@garyp garyp merged commit 6900049 into streem:master Mar 29, 2022
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.

Implement the new proto3 field presence feature
2 participants