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
Add C# namespace to .proto files (e.g. descriptors, unit tests) #348
Conversation
@jtattermusch and @anandolee to review |
@@ -1,7 +1,10 @@ | |||
syntax = "proto2"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without it, protoc issues a warning. Given that it is a proto2 file, it felt reasonable to be explicit. I was somewhat surprised that it was still unspecified, to be honest...
Generally, it looks fine, but this is more for @anandolee to decide. |
Just checked the Travis CI failure, and it's because descriptor.proto has changed but I haven't updated the other files. I'll need to regenerate that on a Linux box... will make the changes in another commit and push that. (It may well be tomorrow...) |
Do we have such a "csharp_namespace" option in our internal code base? If not I suggest we add one first. The .proto files modified in this PR is pulled from our internal code base. We will need to merge all changes here back to google3. |
I'm assuming that we'll merge the entire branch back at some point - I don't know whether any of the C# work has been merged back yet... |
@@ -43,6 +43,8 @@ package google.protobuf; | |||
option java_package = "com.google.protobuf"; | |||
option java_outer_classname = "DescriptorProtos"; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove empty line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Wasn't sure whether we wanted to keep the C# options separate from the Java ones. Will check the other ones in the commit...
I think our current plan is not support c# internally (unless we find some reason to support it), so we don't want to expose csharp options in internal code as well. Discussed with Feng that we should add the ability to remove these options for up integrade. After that, there should be no problem for this Change. |
@anandolee Just to check, are you happy for this change to be pulled into this branch (when Travis is happy, which it should be now... just waiting for confirmation) and we can fix up things internally before the csharp branch is merged into the master branch? |
Yes, go ahead to pull into this branch. I will take care about the internal change before merge into internal code. |
Add C# namespace to .proto files (e.g. descriptors, unit tests)
Instead of using checked out source, this now works with a binary release. Descriptor now has the go_package option set, so we no longer need to move the files around.
No description provided.