andrewhickman / prost-reflect

A protobuf library extending prost with reflection support and dynamic messages.
https://crates.io/crates/prost-reflect
Apache License 2.0
86 stars 19 forks source link

Remove invalid state from Value enum type #51

Closed 124C41p closed 1 year ago

124C41p commented 1 year ago

The enum variants of Value allow constructing certain objects which do not correspond to valid Protobuf data like Lists of Lists or Lists of mixed primitive types:

let invalid1 = Value::List(vec![Value::List(...)])
let invalid2 = Value::List(vec![Value::I32(1), Value::f64(2.345)])

Have you thought about replacing the Value::List variant by various specific variants like Value::I32List(Vec<i32>), Value::MessageList(Vec<DynamicMessage>), etc.?

On the one hand, it is always a good idea to remove invalid state. But more importantly, this change should also improve performance, as it is faster to convert between (say) Vec<i32> and Value if there is a Value::I32List variant, since I don't have to iterate over the vec and map each of its items.

andrewhickman commented 1 year ago

Unfortunately it would become a bit inconsistent having Map(HashMap<MapKey, Value>) but different variants for each list type. I think that this would make the API a lot less ergonomic in a lot of cases as well - if you want to do some processing for every Value instance you have to handle every different list type.