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.

Sign up for GitHub

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

Jump to bottom

Allow FileDescriptors to be parsed with extension registries #8220

Merged
merged 2 commits into from
Feb 1, 2021
Merged

Allow FileDescriptors to be parsed with extension registries #8220

merged 2 commits into from
Feb 1, 2021

Conversation

jskeet
Copy link
Contributor

@jskeet jskeet commented Jan 20, 2021 •
edited
Loading

This allows for much cleaner handling of extensions in dynamic environments such as when writing a protoc plugin.

jskeet added 2 commits January 20, 2021 08:06
@jskeet
Include the size of values (and tags) for extensions, even if the val…
cea3653 
…ue is the default

Fixes protocolbuffers#8218.
@jskeet
Add an extension registry option when parsing file descriptors
f01d9e5 
This is important when parsing descriptor sets that contain extensions.
(The alternative is to get the descriptor bytes again for individual
elements, e.g. message descriptors, and reparse them with the
appropriate extensions. It's really ugly.)
@google-cla google-cla bot added the cla: yes label Jan 20, 2021
@jskeet jskeet requested a review from jtattermusch January 20, 2021 08:46
@jskeet
Copy link
Contributor Author

jskeet commented Jan 20, 2021

(First commit is from #8219.)

@jskeet jskeet added the c# label Jan 20, 2021
jtattermusch
jtattermusch approved these changes Jan 29, 2021
View reviewed changes
Copy link
Contributor

@jtattermusch jtattermusch left a comment

Choose a reason for hiding this comment

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

LGTM, but wondering what the immediate use is. Are you planning to write a protoc plugin in pure C#?

@jskeet
Copy link
Contributor Author

jskeet commented Jan 29, 2021

We already have a protoc plugin in pure C#: https://github.com/googleapis/gapic-generator-csharp
It requires some annoying hackery to get at the options.

Additionally, we're working on an "API index" which will parse protobuf descriptor sets for protos in https://github.com/googleapis/googleapis
Both of these will be simpler when this is merged :)

@hixio-mh
Copy link

#8220
Reviews and change

@jtattermusch
Copy link
Contributor

We already have a protoc plugin in pure C#: https://github.com/googleapis/gapic-generator-csharp
It requires some annoying hackery to get at the options.

Ah, nice, I did not realize that.

Additionally, we're working on an "API index" which will parse protobuf descriptor sets for protos in https://github.com/googleapis/googleapis
Both of these will be simpler when this is merged :)

SG!

@jtattermusch
Copy link
Contributor

I looked at the test failures and they seem unrelated. The Windows C# failure also exists on master.

@jtattermusch jtattermusch merged commit f9e8bf4 into protocolbuffers:master Feb 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@navave navave navave approved these changes

@jtattermusch jtattermusch jtattermusch approved these changes

Assignees
No one assigned
Labels
c# cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jskeet @hixio-mh @jtattermusch @navave @protobuf-kokoro