Navigation Menu

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 ToProto() method to all C# descriptor classes #9426

Merged
merged 1 commit into from Jan 28, 2022

Conversation

jskeet
Copy link
Contributor

@jskeet jskeet commented Jan 20, 2022

Fixes #9425.

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 with one very minor nit.

@@ -68,6 +68,14 @@ internal EnumDescriptor(EnumDescriptorProto proto, FileDescriptor file, MessageD

internal EnumDescriptorProto Proto { get { return proto; } }

/// <summary>
/// Returns a clone of the underlying <see cref="EnumDescriptorProto"/> describing this enum.
/// Note that a copy is taken every time this method is called, so clients using it frequently
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: worth mentioning that a deep clone is taken? (which also explains why it's safe to to .Clone() internally since it provides a deep clone).
Same below.
Leaving resolution up to you - I don't feel strongly about this at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well it specifically says "Returns a clone" and "Note that a copy is taken" - I don't think we need to specifically say it's a deep clone, as a shallow clone would be really odd.

@jskeet jskeet merged commit b926a7d into protocolbuffers:master Jan 28, 2022
@jskeet jskeet deleted the dotnet-reflection-protos branch January 28, 2022 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide (safe) access to reflection protos in C#
3 participants