Repository navigation
Conversation
d7c397b to
7da5c16
Compare
defaulting to proto2 behavior
fd94792 to
a305de2
Compare
| PROTO_2("proto2"), | ||
| PROTO_3("proto3"), | ||
| ; | ||
| /** Syntax and edition version. */ |
There was a problem hiding this comment.
It has nothing to do with your PR, but I just completely hate this design decision in protobuf. What a disaster for authors and readers of protobuf.
| PROTO_3("proto3"), | ||
| ; | ||
| /** Syntax and edition version. */ | ||
| sealed class Syntax(internal val string: String) { |
There was a problem hiding this comment.
I’m tempted to say we create a new thing, Syntax2 or something, that offers these things, and keep Syntax as-is? Making a big binary-incompatible change to wire-runtime has a potentially large user impact?
(Why is Syntax in the runtime API?)
There was a problem hiding this comment.
Because it's passed as a parameter to adapters
expect abstract class ProtoAdapter<E>(
fieldEncoding: FieldEncoding,
type: KClass<*>?,
typeUrl: String?,
syntax: Syntax,
identity: E? = null,
sourceFile: String? = null,
) {| // Created for backward capability with when Syntax used to be an enum. | ||
| fun name(): String { | ||
| return when (this) { | ||
| // TODO(Benoit) Edition needs to return something like `Edition(<value>)`. |
There was a problem hiding this comment.
Yeah, yuck. Anything calling this might not be able to parse it later
There was a problem hiding this comment.
Agreed. name() should not return a constructor expression.
In this draft, Edition("2024").name() returns "PROTO_2", which loses the edition value. Returning "Edition(2024)" would introduce another problem. Java generation inserts it into Syntax.$L.INSTANCE. Kotlin generation uses it as a MemberName. Neither template represents an edition constructor call.
Schema text uses a separate contract. Edition("2024").toString() returns "2024". Syntax.get("2024", edition = true) accepts it. ProtoFileElement.toSchema() writes edition = "2024";. The one-argument Syntax.get rejects "2024". Neither overload accepts "Edition(2024)".
I suggest keeping the proto2 and proto3 name and text forms unchanged. We should agree on an edition name/parsing contract and handle constructor generation separately before replacing this TODO.
| } | ||
|
|
||
| @Test | ||
| fun rejectUnknownEditions() { |
2 participants
defaulting to proto2 behavior, which is kinda wrong.
The way we break the world for Java callers is very sad...
Maybe we could just manually increate the enum values one bye one each year...