protobuf icon indicating copy to clipboard operation
protobuf copied to clipboard

Support annotating fields with field extensions

Open lizziepaquette opened this issue 4 years ago • 5 comments

lizziepaquette avatar Apr 06 '20 22:04 lizziepaquette

Interesting, I implemented a similar feature here: https://github.com/tony612/protobuf-elixir/pull/91/files#diff-b29046f70a5e38390cd6f773e215597cR50

ulissesalmeida avatar Apr 07 '20 15:04 ulissesalmeida

Interesting, I implemented a similar feature here: https://github.com/tony612/protobuf-elixir/pull/91/files#diff-b29046f70a5e38390cd6f773e215597cR50

@ulissesalmeida that is very interesting! This pr was intended to simply expose custom field options in the dsl, but we want to swap out the using protobuf macro https://github.com/tony612/protobuf-elixir/pull/87 and use these field options is to cast types like in gogoproto https://github.com/gogo/protobuf/commit/a2aaeaf7b6dcf98a4a400e026a94713ef90e1ac6#diff-c8ef3ed341f269ec9c5f2f199d371b7b.

One benefit to the field option vs message option approach is that you don't have to go modify the canonical google protos.

Regardless of the implementation though, casting is a extremely useful feature. I agree with your points in #90 and #89, it sucks to keep converting to convenient types in code and would be great if we could configure the compiler to do it for us.

Looking through other prs, #85 is actually extremely similar to this one (I should have checked before). @tony612 left a comment on it detailing that he thinks its better to write supporting compilers than to modify this one: https://github.com/tony612/protobuf-elixir/pull/85#issuecomment-602059403. However with type casting I don't know if that would be truly possible. For example the generated type for the struct will be wrong unless the generator is changed to be aware of these field options.

@ulissesalmeida I would love to know any thoughts you have about moving forward and @tony612 yours as well. Should specifying the generated elixir type for a field be something protobuf-elixir supports?

lizziepaquette avatar Apr 11 '20 13:04 lizziepaquette

@lizziepaquette Thank you for thoughts.

I agree with you about having field types annotation too. Make sense 👍

Now, about plugins vs changing the compiler approach, it's a hard decision, I don't know what's the best.

Having some plugins supports makes sense, or every time we want to add some extension/feature we need to change the original library. It can be very painful for the maintainers. But these plugins should be able to allow a lot of callbacks/hooks.

However, some stuff sounds good having in the core compiler, like a way to annotate your types or automatically transform some types.

I'm very new in this protobuf world, then I did was easier for me that was: change the core library(that is in elixir haha)

ulissesalmeida avatar Apr 13 '20 07:04 ulissesalmeida

@lizziepaquette Thank you for thoughts.

I agree with you about having field types annotation too. Make sense 👍

Now, about plugins vs changing the compiler approach, it's a hard decision, I don't know what's the best.

Having some plugins supports makes sense, or every time we want to add some extension/feature we need to change the original library. It can be very painful for the maintainers. But these plugins should be able to allow a lot of callbacks/hooks.

However, some stuff sounds good having in the core compiler, like a way to annotate your types or automatically transform some types.

I'm very new in this protobuf world, then I did was easier for me that was: change the core library(that is in elixir haha)

This new pr has full fledged type casting, love to hear your thoughts. https://github.com/tony612/protobuf-elixir/pull/100

lizziepaquette avatar Apr 22 '20 20:04 lizziepaquette

@lizziepaquette

Interesting

But I have one question, how a user can extend and add their own types?

This approach is very different from mine here: https://github.com/tony612/protobuf-elixir/pull/91

Correct me if I'm wrong, your approach is based on fields right? Let's say, for this message I want this field to be converted to a string. I believe your changes(field approach) make it more flexible and explicit where you want automatic transformations to happen. However, if you want to make it uniform, you have added those annotations in every place it's needed(can be error-prone).

Meanwhile, my approach is based on messages. Let's say, I want this message to be a String, in the end, no matter where it's used. It's good to make it uniform for all messages in the same app, but for some reason, I want to create exceptions, it's impossible (or harder, I would have to implement it 😆 ).

I think both approaches can live in the core library, but I'm not sure if the library author will want to adopt either haha.

ulissesalmeida avatar Apr 23 '20 08:04 ulissesalmeida