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

Dependable codegen #95

Merged
merged 8 commits into from
Apr 25, 2018
Merged

Dependable codegen #95

merged 8 commits into from
Apr 25, 2018

Conversation

jackalcooper
Copy link
Contributor

@jackalcooper jackalcooper commented Apr 21, 2018

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
Copy link
Owner

tafia commented Apr 21, 2018

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
Copy link
Contributor Author

jackalcooper commented Apr 21, 2018

@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
Copy link
Contributor Author

jackalcooper commented Apr 21, 2018

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)
#97

@tafia
Copy link
Owner

tafia commented Apr 25, 2018

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.

@tafia tafia merged commit 0ae23f6 into tafia:master Apr 25, 2018
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