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

Add C# namespace to .proto files (e.g. descriptors, unit tests) #348

Merged
merged 2 commits into from May 7, 2015

Conversation

jskeet
Copy link
Contributor

@jskeet jskeet commented May 6, 2015

No description provided.

@jskeet
Copy link
Contributor Author

jskeet commented May 6, 2015

@jtattermusch and @anandolee to review

@@ -1,7 +1,10 @@
syntax = "proto2";
Copy link
Contributor

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?

Copy link
Contributor Author

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...

@jtattermusch
Copy link
Contributor

Generally, it looks fine, but this is more for @anandolee to decide.

@jskeet
Copy link
Contributor Author

jskeet commented May 6, 2015

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...)

@xfxyjwf
Copy link
Contributor

xfxyjwf commented May 6, 2015

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.

@jskeet
Copy link
Contributor Author

jskeet commented May 6, 2015

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";

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove empty line

Copy link
Contributor Author

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...

@anandolee
Copy link
Contributor

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.
Just make sure to fix the Travis CI failure.

@jskeet
Copy link
Contributor Author

jskeet commented May 7, 2015

@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?

@anandolee
Copy link
Contributor

Yes, go ahead to pull into this branch. I will take care about the internal change before merge into internal code.
LGTM

jskeet added a commit that referenced this pull request May 7, 2015
Add C# namespace to .proto files (e.g. descriptors, unit tests)
@jskeet jskeet merged commit 34fb666 into protocolbuffers:csharp May 7, 2015
taoso pushed a commit to taoso/protobuf that referenced this pull request Aug 1, 2018
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants