-
Notifications
You must be signed in to change notification settings - Fork 5k
Make converter types public #50192
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
base: main
Are you sure you want to change the base?
Make converter types public #50192
Conversation
Thank you for your contribution @agocke! We will review the pull request and get back to you soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR makes JSON converter types public so they are as accessible as their associated serialized types, enabling source-generated JSON support.
- Changed converter classes from
internal
topublic
- Added XML documentation comments for converters and their methods
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
sdk/resourcemanager/Azure.ResourceManager/src/Resources/Custom/ResourceProviderData.cs | Made ProviderDataConverter public and added docs |
sdk/resourcemanager/Azure.ResourceManager/src/Resources/Custom/Models/WritableSubResource.Serialization.cs | Made WritableSubResourceConverter public and added docs |
sdk/resourcemanager/Azure.ResourceManager/src/Resources/Custom/Models/SubResource.Serialization.cs | Made SubResourceConverter public and added docs |
sdk/resourcemanager/Azure.ResourceManager/src/Common/Custom/Models/ManagedServiceIdentityType.cs | Made ManagedServiceIdentityTypeConverter public and added docs |
sdk/resourcemanager/Azure.ResourceManager/src/Common/Custom/Models/ManagedServiceIdentity.Serialization.cs | Made ManagedServiceIdentityConverter public and added docs |
Comments suppressed due to low confidence (2)
sdk/resourcemanager/Azure.ResourceManager/src/Resources/Custom/ResourceProviderData.cs:58
- Include XML tags for writer, providerData, and options parameters and a tag to ensure consistent and complete API documentation.
public override void Write(Utf8JsonWriter writer, ResourceProviderData providerData, JsonSerializerOptions options)
sdk/resourcemanager/Azure.ResourceManager/src/Resources/Custom/ResourceProviderData.cs:53
- Add unit tests to verify the serialization and deserialization behavior of this public converter to ensure JSON handling works as expected.
public partial class ProviderDataConverter : JsonConverter<ResourceProviderData>
@@ -47,7 +47,7 @@ protected override void JsonModelWriteCore(Utf8JsonWriter writer, ModelReaderWri | |||
if (Optional.IsDefined(Identity)) | |||
{ | |||
writer.WritePropertyName("identity"u8); | |||
JsonSerializer.Serialize(writer, Identity); | |||
JsonSerializer.Serialize(writer, Identity, AzureResourceManagerCosmosDBJsonContext.Default.ManagedServiceIdentity); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should modify this to pass in a common instance of JsonSerializerOptions which uses https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/core/System.ClientModel/src/ModelReaderWriter/JsonModelConverter.cs and specifically calls the ctor that takes in AzureResourceManagerCostmosDBContext.Default
//internal helper somewhere
internal readonly static JsonSerializerOptions s_options = new()
{
Converters =
{
new JsonModelConverter(ModelSerializationExtensions.WireOptions, AzureResourceManagerCosmosDBContext.Default)
}
};
//this line then becomes
JsonSerializer.Serialize(writer, Identity, s_options);
1a6fcec
to
25ce675
Compare
In order to use source-generated JSON, the converter types need to be at least as accessible as the serialized types. This PR changes all public types which have converters to make those converters public.