Skip to content

Support async KeyValuePair (de)serialization - #36607

Merged
layomia merged 3 commits into
dotnet:masterfrom
layomia:kvp
May 28, 2020
Merged

Support async KeyValuePair (de)serialization#36607
layomia merged 3 commits into
dotnet:masterfrom
layomia:kvp

Conversation

@layomia

@layomialayomia commented May 17, 2020•
edited
Loading

Copy link
Copy Markdown
Contributor

Refactors the KeyValuePair converter to use logic for deserializing with parameterized ctors and leverage the async support (for reading and writing).

Fixes#32148 (no longer applicable following this change).

@layomialayomia added this to the 5.0 milestone May 17, 2020
@layomialayomia self-assigned this May 17, 2020
jkotas
jkotas reviewed May 17, 2020
View reviewed changes
Comment threadsrc/libraries/System.Text.Json/src/System.Text.Json.csproj
steveharter
steveharter reviewed May 20, 2020
View reviewed changes
Comment threadsrc/libraries/System.Text.Json/tests/Serialization/OptionsTests.cs
Comment thread...ystem.Text.Json/src/System/Text/Json/Serialization/Converters/Value/KeyValuePairConverter.cs
@jozkee

jozkee commented May 20, 2020

Copy link
Copy Markdown
Member

Do we have stream-based tests with KeyValuePair?

jozkee
jozkee reviewed May 20, 2020
View reviewed changes
Comment threadsrc/libraries/System.Text.Json/tests/Serialization/OptionsTests.cs
@layomia

layomia commented May 22, 2020

Copy link
Copy Markdown
ContributorAuthor

Do we have stream-based tests with KeyValuePair?

Yes, see the diff in Stream.Collections.cs.

layomia
layomia commented May 22, 2020
View reviewed changes
Comment threadsrc/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonValueConverterOfT.cs
{
// Used for value converters that need to re-enter the serializer since it will support JsonPath
// and reference handling.
internal abstract class JsonValueConverter<T> : JsonConverter<T>

@layomialayomiaMay 22, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This converter type may come back depending on work in #36785.

@jaredparjaredpar mentioned this pull request May 22, 2020
Closed
steveharter
steveharter approved these changes May 28, 2020
View reviewed changes

@stevehartersteveharter left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Thanks

@layomia
layomia merged commit 1c5e253 into dotnet:masterMay 28, 2020
@layomia
layomia deleted the kvp branch May 28, 2020 17:15
@ghostghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jkotasjkotasjkotas left review comments
@jozkeejozkeejozkee left review comments
+1 more reviewer
@steveharterstevehartersteveharter approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@layomialayomia

Labels

area-System.Text.Json

Projects

None yet

Milestone

5.0.0

Development

Successfully merging this pull request may close these issues.

The KeyValuePair converter should cache the converters for TKey and TValue

4 participants

@layomia@jozkee@jkotas@steveharter