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

Generate *all* protos in the script, applying fixups. #392

Closed
wants to merge 1 commit into from

Conversation

jskeet
Copy link
Contributor

@jskeet jskeet commented May 15, 2015

We still have some protos which aren't generated how we want them to be:

  • Until we have an option to specify the "umbrella" class, DescriptorProtoFile
    will be broken. (The change of name here affects the reflection descriptor,
    which accounts for most of the change. That's easier than trying to work out
    exactly which occurrences of Descriptor need changing though.)
  • That change affects UnittestCustomOptions
  • Issue Doubly-nested types generate invalid C# code #307 breaks Unittest.cs

After this commit, we don't have the record of the fixups in the files themselves
any more, but one centralized record in the shell script.

The big changes in csharp/src/ProtocolBuffersLite.Test/TestProtos/Unittest.cs is due to reformatting - it's not clear to me why this is different from the rest...

We still have some protos which aren't generated how we want them to be:

- Until we have an option to specify the "umbrella" class, DescriptorProtoFile
  will be broken. (The change of name here affects the reflection descriptor,
  which accounts for most of the change. That's easier than trying to work out
  exactly which occurrences of Descriptor need changing though.)
- That change affects UnittestCustomOptions
- Issue protocolbuffers#307 breaks Unittest.cs

After this commit, we don't have the record of the fixups in the files themselves
any more, but one centralized record in the shell script.
@@ -26,7 +26,12 @@ if [ -z "$PROTOC" ]; then
fi

# Descriptor proto
#TODO(jtattermusch): generate descriptor.proto
# Temporary manual fixup...
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say putting a comment that describes exacly what this temporary fixup is doing would be useful (basically just pasting the text from PR description here).

@jtattermusch
Copy link
Contributor

Besides the comment, LGTM.
But now when all the C# code is in master, I wanted to get rid of the csharp branch entirely, so it would be great if you could recreate this PR to point towards master branch (I will close it now to be able to delete the csharp branch).

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

3 participants