search

tafia / quick-protobuf

A rust implementation of protobuf parser
MIT License
452 stars 87 forks source link

Dependable codegen #95

Closed jackalcooper closed 6 years ago

jackalcooper commented 6 years ago

Thanks for your work on this project, Johann! I am reusing the codegen of quick-protobuf to generate some python code. I have to make this minor changes to make the codegen dependable so I think it is a good idea to merge this PR so that everyone could reuse it in similar scenarios as well.

tafia commented 6 years ago

I am not sure I get the purpose of that change.

Fyi, the protobuf parsing have been extracted into a new crate https://github.com/tafia/protobuf-parser. quick-protobuf will eventually be migrated to this new crate and rust-protobuf has already implemented some pure rust proto parsing using it (optionally).

Do you think your change would fit better in this crate or in quick-protobuf? Again I am not sure how you use that information on your end.

jackalcooper commented 6 years ago

@tafia Thanks for your replay.

My use case is generating the wrapper of python code generated by google protoc. As you might know, code generated by google protoc will keep the directory hierarchy and use it as the module hierarchy of generated python code. That's why I need the info of location of all the imported proto to generate my wrapper.

Do you think your change would fit better in this crate or in quick-protobuf? Probably but it will be great if the two projects are not too "distant"? I mean I'm using a slightly modified main.rs of this project's codegen. I think it is a good idea to keep codegen and parser together because people need to know how to implement a codegen with the parser. The existing codegen for rust would be a good start point.

jackalcooper commented 6 years ago

There is another potential problem of the parser when reusing it. The default field doesn't hold the info of the Enum it comes from. I'm not sure if it is really a problem since proto does have some restriction on the name collision of Enum's member. The typ of a field with a Enum default in a message is FieldType::Message(String) not FieldType::Enum(String) https://github.com/tafia/quick-protobuf/issues/97

tafia commented 6 years ago

Merging for now because there is no reason not to. But I may probably change it all later this year when I'll move to protobuf-parser.

Thanks and sorry for the late answer.