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

Custom types #20

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Custom types #20

wants to merge 6 commits into from

Conversation

cdevienne
Copy link

Add support for custom types in the generated code.

@tiziano88
Copy link
Owner

Thanks @cdevienne , this is an interesting approach, but I have not seen it anywhere else. Are you aware of any other proto plugin doing something similar? In general I think something like this would be useful, but personally I would prefer implementing it via some sort of annotation on the proto files themselves, rather than relying on external files for configuration. What do you think?

@cdevienne
Copy link
Author

cdevienne commented Dec 31, 2018

Hi @tiziano88

I know this approach may seem original, but after working with gogo-protobuf (which add annotations to the protobuf files), I came to think that it is not such a good idea.

I think that the protobuf file only defines a wire protocol and should not impose anything on how a given language should generate code. This decision should be made by the people coding the client program, and they should be able to make this decisions when generating code, without touching the .proto files.

Also, annotating the .proto files for each target language will make the file less readable, and more importantly will make things more complicated for languages not concerned by the annotations. For example, having gogo annotations when generating python code is such a pain that I had to generate gogo-less versions of my .proto files...

This is why I had this new approach for elm-protobuf, which I found a lot nicer. The yaml file could be a different format (json for example), or have a different, more intuitive structure, I am all ears for suggestions.

Let me know what you thing about it.

(Note: fyi I am also coding, on top of elm-protobuf and elm-nats, a elm-nrpc package)

@tiziano88
Copy link
Owner

Thanks for the clarification, it is interesting to hear your point of view! I think I agree with you that using annotations on proto files will become overly verbose, especially if they need to be maintained for several target languages. I am afraid that introducing a YAML file with extra metadata will be a slippery slope to adding all kind of random stuff later on. In particular, it seems that converting individual files to more specific representation is effectively an orthogonal post-processing step that may be applied after the fact to the generated proto structs; this may mean manually creating a separate, more expressive type with the desired semantics, and converters to and from the generated struct, or perhaps create a separate generator (possibly based on YAML, like in your example) that takes an arbitrary struct, and produces a new one with the intended semantics and the corresponding converters. That way it can be more generic and not specifically tied to elm-protobuf.

@cdevienne
Copy link
Author

If I understand correctly, you suggest adding an extra step to modify the generated structs, that could actually be a refactoring tool for elm in general?

Interesting, but it seems much more complex to me than just customize the output of a plugin with a few metadata.

I think the goal of generating code is only to avoid writing it, not avoid thinking it, which means the user should be able to make some choices (within a reasonable scope).

I get that having a metadata file can lead to abuses in the future, but it remains the simplest approach I can think of, and we can always be careful what options we accept to include.

We could also make the metadata format generic enough so it can be used by other plugins, and resemble to what is already one with annotations.

This option will dynamically add files to the excludedFiles list, allowing proto2 files in the
dependencies of a proto3 file as long as its data structures are not used directly.
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