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 MessageType and EnumType on FieldDescriptorProto #971

Merged
merged 2 commits into from Oct 30, 2022

Conversation

rayokota
Copy link
Contributor

@mgravell , I'm looking for a .proto parser that I can use for https://github.com/confluentinc/confluent-kafka-dotnet/

It appears that the one in protobuf-net.Reflection is close, but I cannot seem to find a way to get the corresponding DescriptorProto for a given FieldDescriptorProto that references the message type.

I believe this PR will help, and also fixes #532

@mgravell
Copy link
Member

In principle, I think this is fine. Thoughts:

  1. These are mutually exclusive and different types; we should be able to overlap the two values - maybe a single object or IType field (probably the second), and use theField as EnumDescriptorProto; (or whatever) for the setter?
  2. Needs a test to show that they work as intended in at least one scrnario

@mgravell
Copy link
Member

mgravell commented Oct 28, 2022

A small part of me is worried about conflicts on the API related to schema additions. I've been careful not to pollute the public API of these types for that reason. I wonder if what we should do here is something like:

internal IType UnderlyingType {get;set;}
// or ResolvedType - naming is hard!

on the field, and have separately some extension methods:

public static DescriptorExtensions
{
    public static EnumDescriptorProto GetEnumType(this FieldDescriptorProto field)
        => field?.UnderlyingType as EnumDescriptorProto;
    // etc
}

This guarantees that we're not hosed if Google ever add conflicting members. Thoughts?

@rayokota
Copy link
Contributor Author

rayokota commented Oct 28, 2022

A small part of me is worried about conflicts on the API related to schema additions. I've been careful not to pollute the public API of these types for that reason.

Yes, I like your approach of using extension methods very much. I'll give it a shot when I get a chance, unless you want to do it first :)

@rayokota
Copy link
Contributor Author

@mgravell , I made the changes as you requested. Please take a look. Thanks!

@mgravell
Copy link
Member

Rerunning tests; SO18695728 failed but that is unrelated to your changes, so presumably something unstable in there that I need to fix separately. Will merge if test re-run voodoos that away.

Thanks; nice changes; it shouldn't be part of this PR, but: I wonder if we can simplify some of the code-gen code by using this new member.

@mgravell
Copy link
Member

Actions on me:

  1. Investigate flakey SO18695728
  2. Investigate using new member in code-gen (removes dictionary?)

@mgravell mgravell merged commit a0fb547 into protobuf-net:main Oct 30, 2022
@rayokota
Copy link
Contributor Author

Awesome, thanks for the quick turnaround! 👍

@rayokota rayokota deleted the add-field-type branch October 30, 2022 16:37
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.

API additions
2 participants