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

Allow .proto3 file extensions #265

Closed
wants to merge 1 commit into from
Closed

Allow .proto3 file extensions #265

wants to merge 1 commit into from

Conversation

nhynes
Copy link
Contributor

@nhynes nhynes commented Feb 2, 2018

Currently rust_protoc cannot compile onnx.proto3 but for no other reason than it ends with .proto3 instead of .proto. This PR remedies this by also trying to remove a .proto3 extension.

@stepancheg
Copy link
Owner

stepancheg commented Feb 7, 2018

Thank you for the patch.

I think rust-protobuf should follow protoc behavior when generating code. When called for file named onnx.proto3, protoc generates:

For C++ it simply appends .h suffix:
onnx.proto3.pb.h for C++

For C# and Ruby it strips the suffix:
Onnx.cs for C#
onnx_pb.rb for Ruby:

For Java and Python it doesn't attempt to strip any suffix except .proto.

OnnxProto3.java for Java
onnx/proto3_pb2.py for Python

Anyway, my point is rust-protobuf should match behavior of Google's protobuf compiler:

  • handle any file name suffix, not just .proto or .proto3
  • stripped unknown suffix should be either appended to file name (as for Java) or ignored (as for C#) (I don't know what would be better)

If you wish to submit another patch, I would be happy to merge it. Note, patch should also include a test. Since it involves code generation based on file name, test should live somewhere inside protobuf-test directory.

@nhynes
Copy link
Contributor Author

nhynes commented Feb 8, 2018

@stepancheg thanks for the comments! I took your advice to leave the unrecognized extension alone. Regarding testing, I updated the unit tests in strx.rs since that file contained the only non-trivial changes.

@stepancheg
Copy link
Owner

I guess with your new patch abc.proto3 will result in generated abc_proto3.rs which is good thing.

Test should be patched protobuf-test module. I can write a test myself, but it will take some time.

@nhynes
Copy link
Contributor Author

nhynes commented Feb 8, 2018

I added unit tests to descriptorx.rs. I don't think that integration tests are necessary, actually.

@stepancheg
Copy link
Owner

Integration tests are necessary, so I had to write them myself.

Thank you for the patch! Merged. #268

@stepancheg stepancheg closed this Feb 14, 2018
@nhynes nhynes deleted the proto3-ext branch February 14, 2018 01:20
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