Conversation
Signed-off-by: Sam Gammon <sam@elide.ventures>
|
cc / @Dogacel |
There was a problem hiding this comment.
Hi 👋 First of all, thank you for contributing!
It seems like some of the changes you made caused field signatures to change. I see issues especially with nullability. Even though we use a newer version of protobuf compiler, the existing generated code shouldn't change because it's syntax is proto3.
Also, it would be nice to include some proto2 and edition 2023 test files.
According to https://protobuf.dev/editions/implementation/ we still have a lot to support.
| ) { | ||
| init { | ||
| require( | ||
| listOfNotNull( | ||
| someValue, | ||
| ).size <= 1 | ||
| ) { "Should only contain one of some_oneof." } | ||
| } | ||
| } |
There was a problem hiding this comment.
Hmm, I wonder whether isSyntheticCompat works as intended in this situation or not. I don't see a reason why this block would be removed. It is not possible to set more than 1 one ofs.
| // Indicate whether the field unconditionally has a value; true if the field is PROTO2 and marked `required`, or if | ||
| // the field has a default value when unset. | ||
| private val FieldDescriptor.unconditionallyHasValue: Boolean get() = | ||
| !isOptional || !options.features.hasFieldPresence() |
There was a problem hiding this comment.
I am not sure if this works as intended.
In proto 3, having field presence = being marked as optional.
The method name was a little confusing, maybe we should name that something like hasFieldPresence()?
There was a problem hiding this comment.
Also I wonder whether optional int32 a = 1 in proto3 and int32 a = 1 in edition 2023 both have hasFieldPresence set to true. Can you cross-check, write a test and confirm?
|
|
||
| if ( | ||
| fieldDescriptor.hasOptionalKeyword() || | ||
| fieldDescriptor.isOptional || |
There was a problem hiding this comment.
I think this causes a bug, we should update this to be unconditionallyHasValue as well.
There was a problem hiding this comment.
Or maybe hasFieldPresence() is enough. Let's give it a try.
|
@sgammon hi, just checking in as this PR has became stale. |
|
@Dogacel Sorry, got busy with work... we don't need this component quite yet, but we will soon. Thank you for your review comments. Why don't I re-file this week after some cleanup? 😄 |
Sure! Waiting for it 🙂 |
|
@sgammon I have released a version I don't have time to write code to cover proto2, proto2_editions, proto3_editions and editions2023. However I have added some conformance test classes and fixed a couple-bugs on the go. I am thinking about using https://github.com/dogacel/kotest-property-protobuf to test the serialization feature as well. |
2 participants
Hey! Cool project. I'd like to contribute and potentially use it in some of my work. Can I help contribute? 😄
First up, here is a PR which updates Protobuf to the latest version (
4.28.1at the time of this writing). It includes some transition internally to Protocol Buffer Editions, which happen to provide a nicer API for some of the stuff needed by this plugin.I've updated in the places I think need it, and done a little clean up along the way. Please make sure, though, that I've done this right according to the codebase's styles and so on. The tests pass and we intend to use the lib downstream. Cheers!