open-toast / protokt

Protobuf compiler and runtime for Kotlin
Apache License 2.0
153 stars 14 forks source link

Add an option to use converters for all well known types #278

Open friscoMad opened 2 months ago

friscoMad commented 2 months ago

I love the converters support in this lib but it would be nice to have an option to use the wrappers that you provide as an extension, in custom wrappers (like UUID) it makes sense to define it at the field level but if a team want to use the converters every time for timestamps they need to add the option to every single field which is not a great DevEx. I understand this should not be enforced but having the option would be nice.

An alternative would be to provide an option to define a map<Type,Converter> to be applied during code generation, with that solution it will cover well-known types and more special cases like an API where all bytes should be UUIDs.

andrewparmet commented 1 month ago

I understand the friction of having to specify the wrapper option every time, and I've seen other code generator libraries use type mappings for this sort of thing. I hesitate to introduce that functionality for a few reasons:

friscoMad commented 1 month ago

First of all thanks for the in deep reply. Yes, I was thinking of a protoc option that gets passed to the plugin, but you are right and I was not thinking about the conversion to DynamicMessage feature. However, it should be possible to keep it working if you add the option artificially in the generated protokt descriptors when the option is set.

That said I have been working on a workaround implementing a descriptor set preprocessor that does exactly that, it adds an extra step, and sadly the gradle Protobuf plugin does not yet support the descriptors_in option but I think we can use the workaround if this is not implemented.