Enable configuring dynamic behavior through client options #36868
Conversation
|
API change check APIView has identified API level changes in this PR and created following API reviews. |
| @@ -1145,6 +1136,11 @@ public abstract partial class ObjectSerializer | |||
| public abstract System.Threading.Tasks.ValueTask SerializeAsync(System.IO.Stream stream, object? value, System.Type inputType, System.Threading.CancellationToken cancellationToken); | |||
| public virtual System.Threading.Tasks.ValueTask<System.BinaryData> SerializeAsync(object? value, System.Type? inputType = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; } | |||
| } | |||
| public partial class RawContentOptions | |||
There was a problem hiding this comment.
This will expand to include date/time options in a subsequent PR
| public partial class RawContentOptions | ||
| { | ||
| public RawContentOptions() { } | ||
| public bool UseCamelCaseNamingConvention { get { throw null; } set { } } |
There was a problem hiding this comment.
I wanted to show this way of naming it to encourage discussion. I think it's probably better if we make what we're now calling PropertyNamingConvention a public enum and use those values to set this property. This would let us use the PropertyNamingConvention enum in CosmosSerializationOptions and improve extensibility if we wanted to add other naming convention policies out of the box later.
If we did this, I would probably remove the public construct from this type, and change the overloads that take RawContentOptions to take those enum values directly instead, but open to discussion.
There was a problem hiding this comment.
To give credit where due, this name comes from EntityFramework.
| @@ -172,7 +174,9 @@ private IEnumerable GetEnumerable() | |||
| value = ConvertType(value); | |||
| } | |||
|
|
|||
| if (_options.CaseMapping == DynamicCaseMapping.PascalToCamel) | |||
| // TODO: implement check for existing property before writing converted | |||
There was a problem hiding this comment.
I'll still update this before this PR merges.
| @@ -44,6 +46,13 @@ public abstract class Response : IDisposable | |||
| // TODO(matell): The .NET Framework team plans to add BinaryData.Empty in dotnet/runtime#49670, and we can use it then. | |||
| private static readonly BinaryData s_EmptyBinaryData = new BinaryData(Array.Empty<byte>()); | |||
|
|
|||
| private DynamicDataOptions _dynamicOptions = DynamicDataOptions.Default; | |||
There was a problem hiding this comment.
I'll probably shuffle this around to hold RawContentOptions instead.
...gnitivelanguage/Azure.AI.Language.Conversations/tests/ConversationAnalysisClientLiveTests.cs
Show resolved
Hide resolved
| @@ -381,6 +380,7 @@ public abstract partial class ClientOptions | |||
| protected ClientOptions(Azure.Core.DiagnosticsOptions? diagnostics) { } | |||
| public static Azure.Core.ClientOptions Default { get { throw null; } } | |||
| public Azure.Core.DiagnosticsOptions Diagnostics { get { throw null; } } | |||
| public Azure.Core.Serialization.RawContentOptions RawContent { get { throw null; } } | |||
There was a problem hiding this comment.
What if some properties in the schema as pascalcased and some camelcased? If we stored the schema in the client, it could handle such cases and there would not be a need for the user to set any options.
There was a problem hiding this comment.
What if some properties in the schema as pascalcased and some camelcased?
If that's the case, I would recommend the user not pass a configuration option to change the casing but instead use the default "strict-casing" setting to handle this precisely. Or, if there was a reason to set PropertyNamingConvention.CamelCase, they could use the indexer syntax to override any magic we're doing.
If we stored the schema in the client, it could handle such cases and there would not be a need for the user to set any options.
It's true. I think taking this approach sets us up to add that feature in the future, if we decide it was worth the investment. I'm not sure we'd want to push out the release schedule to work on this feature before releasing the type, though. Let me know if you feel strongly otherwise.
|
Closing in favor of #36887. For some reason, new commits aren't being added to this PR. |
Addresses #36642