Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support annotating fields with field extensions #94

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

lizziepaquette
Copy link

No description provided.

@ulissesalmeida
Copy link
Contributor

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

lizziepaquette and others added 2 commits April 8, 2020 12:17
* usurp using macro

* address missed message types and fix Elixir.Protobuf problem

* fix spacing
@lizziepaquette
Copy link
Author

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 #87 and use these field options is to cast types like in gogoproto gogo/protobuf@a2aaeaf#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: #85 (comment). 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 lizziepaquette force-pushed the support-field-extensions branch from a0ed603 to 13438ac Compare April 12, 2020 19:41
@ulissesalmeida
Copy link
Contributor

@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)

@lizziepaquette
Copy link
Author

@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. #100

@ulissesalmeida
Copy link
Contributor

ulissesalmeida commented Apr 23, 2020

@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:
#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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants