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

C#: Mark GetOption API as obsolete and expose the "GetOptions()" method on descriptors instead #7434

Merged
merged 8 commits into from May 11, 2020
Merged

C#: Mark GetOption API as obsolete and expose the "GetOptions()" method on descriptors instead #7434

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
944055a
expose Options property on descriptors
jtattermusch Apr 28, 2020
9720ae4
adjust CustomOptionsTest.cs
jtattermusch Apr 28, 2020
f4ff220
add a few more tests
jtattermusch Apr 28, 2020
062c2dc
add back GetOption methods, but mark them as obsolete
jtattermusch Apr 28, 2020
402ce8b
add back GetOption tests
jtattermusch Apr 28, 2020
673245f
remove unnecessary cast
jtattermusch Apr 29, 2020
ff999c9
clarify docs
jtattermusch Apr 29, 2020
0688883
address review comments
jtattermusch May 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
150 changes: 88 additions & 62 deletions csharp/src/Google.Protobuf.Test/Reflection/CustomOptionsTest.cs
View file
Open in desktop

Large diffs are not rendered by default.

11 changes: 10 additions & 1 deletion csharp/src/Google.Protobuf/Reflection/EnumDescriptor.cs
View file
Open in desktop
Expand Up @@ -128,12 +128,20 @@ public EnumValueDescriptor FindValueByName(string name)
/// <summary>
/// The (possibly empty) set of custom options for this enum.
/// </summary>
[Obsolete("CustomOptions are obsolete. Use GetOption")]
[Obsolete("CustomOptions are obsolete. Use the Options property.")]
public CustomOptions CustomOptions => new CustomOptions(Proto.Options?._extensions?.ValuesByNumber);

/// <summary>
Copy link
Contributor

@JamesNK JamesNK Apr 28, 2020 •
edited

Choose a reason for hiding this comment

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

Can null ever be returned?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Improved the docs.

/// The <c>EnumOptions</c>, defined in <c>descriptor.proto</c>.
/// Custom options can be retrieved as extensions of the returned message.
/// NOTE: A defensive copy is created each time this property is retrieved.
/// </summary>
public EnumOptions Options => (Proto.Options as IDeepCloneable<EnumOptions>)?.Clone();
Copy link
Contributor

@JamesNK JamesNK Apr 28, 2020 •
edited

Choose a reason for hiding this comment

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

Can Proto.Options on an EnumDescriptor ever not be IDeepCloneable<EnumOptions>? Same question for all the other as casts. I prefer to do a regular cast unless there is a reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, removed the cast.


/// <summary>
/// Gets a single value enum option for this descriptor
/// </summary>
[Obsolete("GetOption is obsolete. Use the Options property.")]
public T GetOption<T>(Extension<EnumOptions, T> extension)
{
var value = Proto.Options.GetExtension(extension);
Expand All @@ -143,6 +151,7 @@ public T GetOption<T>(Extension<EnumOptions, T> extension)
/// <summary>
/// Gets a repeated value enum option for this descriptor
/// </summary>
[Obsolete("GetOption is obsolete. Use the Options property.")]
public RepeatedField<T> GetOption<T>(RepeatedExtension<EnumOptions, T> extension)
{
return Proto.Options.GetExtension(extension).Clone();
Expand Down
11 changes: 10 additions & 1 deletion csharp/src/Google.Protobuf/Reflection/EnumValueDescriptor.cs
View file
Open in desktop
Expand Up @@ -73,12 +73,20 @@ public sealed class EnumValueDescriptor : DescriptorBase
/// <summary>
/// The (possibly empty) set of custom options for this enum value.
/// </summary>
[Obsolete("CustomOptions are obsolete. Use GetOption")]
[Obsolete("CustomOptions are obsolete. Use the Options property")]
public CustomOptions CustomOptions => new CustomOptions(Proto.Options?._extensions?.ValuesByNumber);

/// <summary>
/// The <c>EnumValueOptions</c>, defined in <c>descriptor.proto</c>.
/// Custom options can be retrieved as extensions of the returned message.
/// NOTE: A defensive copy is created each time this property is retrieved.
/// </summary>
public EnumValueOptions Options => (Proto.Options as IDeepCloneable<EnumValueOptions>)?.Clone();

/// <summary>
/// Gets a single value enum value option for this descriptor
/// </summary>
[Obsolete("GetOption is obsolete. Use the Options property.")]
public T GetOption<T>(Extension<EnumValueOptions, T> extension)
{
var value = Proto.Options.GetExtension(extension);
Expand All @@ -88,6 +96,7 @@ public T GetOption<T>(Extension<EnumValueOptions, T> extension)
/// <summary>
/// Gets a repeated value enum value option for this descriptor
/// </summary>
[Obsolete("GetOption is obsolete. Use the Options property.")]
public RepeatedField<T> GetOption<T>(RepeatedExtension<EnumValueOptions, T> extension)
{
return Proto.Options.GetExtension(extension).Clone();
Expand Down
11 changes: 10 additions & 1 deletion csharp/src/Google.Protobuf/Reflection/FieldDescriptor.cs
View file
Open in desktop
Expand Up @@ -304,12 +304,20 @@ public MessageDescriptor ExtendeeType
/// <summary>
/// The (possibly empty) set of custom options for this field.
/// </summary>
[Obsolete("CustomOptions are obsolete. Use GetOption")]
[Obsolete("CustomOptions are obsolete. Use the Options property.")]
public CustomOptions CustomOptions => new CustomOptions(Proto.Options?._extensions?.ValuesByNumber);

/// <summary>
/// The <c>FieldOptions</c>, defined in <c>descriptor.proto</c>.
/// Custom options can be retrieved as extensions of the returned message.
/// NOTE: A defensive copy is created each time this property is retrieved.
/// </summary>
public FieldOptions Options => (Proto.Options as IDeepCloneable<FieldOptions>)?.Clone();

/// <summary>
/// Gets a single value field option for this descriptor
/// </summary>
[Obsolete("GetOption is obsolete. Use the Options property.")]
public T GetOption<T>(Extension<FieldOptions, T> extension)
{
var value = Proto.Options.GetExtension(extension);
Expand All @@ -319,6 +327,7 @@ public T GetOption<T>(Extension<FieldOptions, T> extension)
/// <summary>
/// Gets a repeated value field option for this descriptor
/// </summary>
[Obsolete("GetOption is obsolete. Use the Options property.")]
public RepeatedField<T> GetOption<T>(RepeatedExtension<FieldOptions, T> extension)
{
return Proto.Options.GetExtension(extension).Clone();
Expand Down
11 changes: 10 additions & 1 deletion csharp/src/Google.Protobuf/Reflection/FileDescriptor.cs
View file
Open in desktop
Expand Up @@ -547,12 +547,20 @@ public override string ToString()
/// <summary>
/// The (possibly empty) set of custom options for this file.
/// </summary>
[Obsolete("CustomOptions are obsolete. Use GetOption")]
[Obsolete("CustomOptions are obsolete. Use the Options property.")]
public CustomOptions CustomOptions => new CustomOptions(Proto.Options?._extensions?.ValuesByNumber);

/// <summary>
/// The <c>FileOptions</c>, defined in <c>descriptor.proto</c>.
/// Custom options can be retrieved as extensions of the returned message.
/// NOTE: A defensive copy is created each time this property is retrieved.
/// </summary>
public FileOptions Options => (Proto.Options as IDeepCloneable<FileOptions>)?.Clone();

/// <summary>
/// Gets a single value file option for this descriptor
/// </summary>
[Obsolete("GetOption is obsolete. Use the Options property.")]
public T GetOption<T>(Extension<FileOptions, T> extension)
{
var value = Proto.Options.GetExtension(extension);
Expand All @@ -562,6 +570,7 @@ public T GetOption<T>(Extension<FileOptions, T> extension)
/// <summary>
/// Gets a repeated value file option for this descriptor
/// </summary>
[Obsolete("GetOption is obsolete. Use the Options property.")]
public RepeatedField<T> GetOption<T>(RepeatedExtension<FileOptions, T> extension)
{
return Proto.Options.GetExtension(extension).Clone();
Expand Down
11 changes: 10 additions & 1 deletion csharp/src/Google.Protobuf/Reflection/MessageDescriptor.cs
View file
Open in desktop
Expand Up @@ -287,12 +287,20 @@ internal bool IsExtensionsInitialized(IMessage message)
/// <summary>
/// The (possibly empty) set of custom options for this message.
/// </summary>
[Obsolete("CustomOptions are obsolete. Use GetOption")]
[Obsolete("CustomOptions are obsolete. Use the Options property")]
public CustomOptions CustomOptions => new CustomOptions(Proto.Options?._extensions?.ValuesByNumber);

/// <summary>
/// The <c>MessageOptions</c>, defined in <c>descriptor.proto</c>.
/// Custom options can be retrieved as extensions of the returned message.
/// NOTE: A defensive copy is created each time this property is retrieved.
/// </summary>
public MessageOptions Options => (Proto.Options as IDeepCloneable<MessageOptions>)?.Clone();

/// <summary>
/// Gets a single value message option for this descriptor
/// </summary>
[Obsolete("GetOption is obsolete. Use the Options property.")]
public T GetOption<T>(Extension<MessageOptions, T> extension)
{
var value = Proto.Options.GetExtension(extension);
Expand All @@ -302,6 +310,7 @@ public T GetOption<T>(Extension<MessageOptions, T> extension)
/// <summary>
/// Gets a repeated value message option for this descriptor
/// </summary>
[Obsolete("GetOption is obsolete. Use the Options property.")]
public Collections.RepeatedField<T> GetOption<T>(RepeatedExtension<MessageOptions, T> extension)
{
return Proto.Options.GetExtension(extension).Clone();
Expand Down
11 changes: 10 additions & 1 deletion csharp/src/Google.Protobuf/Reflection/MethodDescriptor.cs
View file
Open in desktop
Expand Up @@ -73,12 +73,20 @@ public sealed class MethodDescriptor : DescriptorBase
/// <summary>
/// The (possibly empty) set of custom options for this method.
/// </summary>
[Obsolete("CustomOptions are obsolete. Use GetOption")]
[Obsolete("CustomOptions are obsolete. Use the Options property.")]
public CustomOptions CustomOptions => new CustomOptions(Proto.Options?._extensions?.ValuesByNumber);

/// <summary>
/// The <c>MethodOptions</c>, defined in <c>descriptor.proto</c>.
/// Custom options can be retrieved as extensions of the returned message.
/// NOTE: A defensive copy is created each time this property is retrieved.
/// </summary>
public MethodOptions Options => (Proto.Options as IDeepCloneable<MethodOptions>)?.Clone();

/// <summary>
/// Gets a single value method option for this descriptor
/// </summary>
[Obsolete("GetOption is obsolete. Use the Options property.")]
public T GetOption<T>(Extension<MethodOptions, T> extension)
{
var value = Proto.Options.GetExtension(extension);
Expand All @@ -88,6 +96,7 @@ public T GetOption<T>(Extension<MethodOptions, T> extension)
/// <summary>
/// Gets a repeated value method option for this descriptor
/// </summary>
[Obsolete("GetOption is obsolete. Use the Options property.")]
public RepeatedField<T> GetOption<T>(RepeatedExtension<MethodOptions, T> extension)
{
return Proto.Options.GetExtension(extension).Clone();
Expand Down
11 changes: 10 additions & 1 deletion csharp/src/Google.Protobuf/Reflection/OneofDescriptor.cs
View file
Open in desktop
Expand Up @@ -117,12 +117,20 @@ public MessageDescriptor ContainingType
/// <summary>
/// The (possibly empty) set of custom options for this oneof.
/// </summary>
[Obsolete("CustomOptions are obsolete. Use GetOption")]
[Obsolete("CustomOptions are obsolete. Use the Options property.")]
public CustomOptions CustomOptions => new CustomOptions(proto.Options?._extensions?.ValuesByNumber);

/// <summary>
/// The <c>OneofOptions</c>, defined in <c>descriptor.proto</c>.
/// Custom options can be retrieved as extensions of the returned message.
/// NOTE: A defensive copy is created each time this property is retrieved.
/// </summary>
public OneofOptions Options => (proto.Options as IDeepCloneable<OneofOptions>)?.Clone();

/// <summary>
/// Gets a single value oneof option for this descriptor
/// </summary>
[Obsolete("GetOption is obsolete. Use the Options property.")]
public T GetOption<T>(Extension<OneofOptions, T> extension)
{
var value = proto.Options.GetExtension(extension);
Expand All @@ -132,6 +140,7 @@ public T GetOption<T>(Extension<OneofOptions, T> extension)
/// <summary>
/// Gets a repeated value oneof option for this descriptor
/// </summary>
[Obsolete("GetOption is obsolete. Use the Options property.")]
public RepeatedField<T> GetOption<T>(RepeatedExtension<OneofOptions, T> extension)
{
return proto.Options.GetExtension(extension).Clone();
Expand Down
11 changes: 10 additions & 1 deletion csharp/src/Google.Protobuf/Reflection/ServiceDescriptor.cs
View file
Open in desktop
Expand Up @@ -94,12 +94,13 @@ public MethodDescriptor FindMethodByName(String name)
/// <summary>
/// The (possibly empty) set of custom options for this service.
/// </summary>
[Obsolete("CustomOptions are obsolete. Use GetOption")]
[Obsolete("CustomOptions are obsolete. Use the Options property.")]
public CustomOptions CustomOptions => new CustomOptions(Proto.Options?._extensions?.ValuesByNumber);

/// <summary>
/// Gets a single value service option for this descriptor
/// </summary>
[Obsolete("GetOption is obsolete. Use the Options property.")]
public T GetOption<T>(Extension<ServiceOptions, T> extension)
{
var value = Proto.Options.GetExtension(extension);
Expand All @@ -109,11 +110,19 @@ public T GetOption<T>(Extension<ServiceOptions, T> extension)
/// <summary>
/// Gets a repeated value service option for this descriptor
/// </summary>
[Obsolete("GetOption is obsolete. Use the Options property.")]
public RepeatedField<T> GetOption<T>(RepeatedExtension<ServiceOptions, T> extension)
{
return Proto.Options.GetExtension(extension).Clone();
}

/// <summary>
/// The <c>ServiceOptions</c>, defined in <c>descriptor.proto</c>.
/// Custom options can be retrieved as extensions of the returned message.
/// NOTE: A defensive copy is created each time this property is retrieved.
/// </summary>
public ServiceOptions Options => (Proto.Options as IDeepCloneable<ServiceOptions>)?.Clone();

internal void CrossLink()
{
foreach (MethodDescriptor method in methods)
Expand Down