-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Update to serialize / deserialize BinaryData to support untyped yet dynamic used cases #27382
Conversation
Latest profile
|
API changes have been detected in API changes + public static Dictionary<string, object?> ToDictionaryFromJson(this BinaryData data); |
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.
Would you be willing to add a description to the PR sharing what motivates this change, and the context behind it? In the past we haven't used Dictionary APIs in .NET, so I'm curious what changed in our thinking about this.
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.
Changes to Cognitive Language and KV LGTM, but I do not think we should be returning a concrete type. I see no benefit over an IDictionary<string, object>
, though I appreciate that in this case with an extension method it's not as important as with model properties or other mockable members.
API changes have been detected in API changes + public static IDictionary<string, object?> ToDictionary(this BinaryData data); |
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.
I thought we agreed to ship the slower implementation now and optimize it later. I think it's a better option given we want to GA Azure.Core in the next couple of days and the slow implementation is much simpler.
This was the slower one, it doesn't include any of the optimizations we talked about that are needed in BCL. I did show one other straw man in our meeting that double parsed when you accessed which was over 3x slower and 2.5x allocs do you mean use that one? |
New perf profile after switching to JsonElement implementation
|
API changes have been detected in API changes + public static IDictionary<string, object?> ToDictionaryFromJson(this BinaryData data); |
Co-authored-by: JoshLove-msft <[email protected]>
Contributing to the Azure SDK
In mgmt plane we have some APIs like the
GenericResource
that return a partially strongly typed model. In the past the serializer would model this asobject
but would instantiate it as a Dictionary<string, object>. The issue was that customers didn't know what to do with an object and googling "How to use object in Azure" was fruitless. We want to instead model this asBinaryData
and then provide samples for customers on the 4 different ways to load data in but still give them the ability to use it on output as aDictionary<string, object>
.For deserializing here is an example of how they would use this.
The csproj files happened because as part of this there are a few more files that are used when you include LRO in your project so they were moved into the common definition of
IncludeOperationsSharedSource
Please see our CONTRIBUTING.md if you are not familiar with contributing to this repository or have questions.
For specific information about pull request etiquette and best practices, see this section.