Open mzabaluev opened 4 months ago
Please add a description that explains your proposed change. That will make reviewing easier.
Do you think that this logic could be applied to the oneof field ?
Do you think that this logic could be applied to the oneof field ?
I believe it works differently for oneofs: if an unknown field number is encountered in a message and it's not a known oneof variant or a regular field, the field is ignored. So the oneof field that would get the value if the variant field were described in the proto would get None
instead.
Will this break compatibility with proto2/closed enums? Personally, I don't use proto2, so I am not sure whether it is properly supported at all.
I think it makes sense to provide a migration guide.
What is the error message for a i32
field with #[prost(enumeration)]
? Can we detect that scenario and print a nice error message?
I would like to see some tests for OpenEnum
.
Will this break compatibility with proto2/closed enums? Personally, I don't use proto2, so I am not sure whether it is properly supported at all.
I believe prost has never been conformant with closed enums: the raw values have been left in the message as is. I think we could try to support closed enums in two ways:
OpenEnum
, but in the closed enum case, decode unknown values as Known(Default::default())
. This leaves the possibility of producing unknown values on encoding.Default::default()
.I prefer option 2, even though it requires more work. Protobuf edition 2023 has closed enums as a feature, so they need to be supported even if we ignore proto2.
I think it makes sense to provide a migration guide.
What would be a good place for it? I can add a section to the README.
I would like to see some tests for
OpenEnum
.
I'm going to add at least one good example/doctest on the type.
Have thought some more about this PR: I don't want to break users in this way. At least not now.
I suggest making this an option in prost-build
so that we can experiment with the API without breaking existing users. That way, interested users can opt in, and we don't have to create a perfect OpenEnum
API on the first try.
Once we feel good about the new API, we can think about changing the default.
Please look at bytes for a good example of changing the generated data type.
Instead of introducing a new type, a simple Result<T, i32>
sounds more logical to me.
enum-repr-derive
follows this approach when parsing an enum from i32
Instead of introducing a new type, a simple
Result<T, i32>
sounds more logical to me.
It's weird to have a Result
as a struct field. The names of variants and methods of Result
are less than intuitively applicable: it's not necessarily an error to receive an unknown enum value from the wire, so we should give the API users a speed bump to decide how to deal with them. There are convenience methods and the TryInto
impl to convert the OpenEnum
value to a Result
if that's the chosen approach.
Another benefit of introducing a new type is for the add-on macros and code generators that derive something on structs generated by prost-build. These could deal with the OpenEnum
type in some specific way, even without access to the proto descriptor data for the field. Doing the same (e.g. defining generic trait impls) for Result
would feel like too much overloading.
How are default values handles in this solution? Especially default values set for a specific field in the proto file.
Another solution for #276, amenable to destructuring.
This changes the representation of enum fields in message structs to this generic wrapper, parameterized over the generated known enum type:
In an improvement over #1061, this allows convenient matching of enum field values as part of the message. There are also convenience methods to fallibly extract the known value in an
Option
orResult
.