Skip to content

Add JsonSerializerOptions.Default - #61434

Merged
eiriktsarpalis merged 3 commits into
dotnet:mainfrom
eiriktsarpalis:feature/jsonserializeroptions-default
Nov 15, 2021
Merged

Add JsonSerializerOptions.Default#61434
eiriktsarpalis merged 3 commits into
dotnet:mainfrom
eiriktsarpalis:feature/jsonserializeroptions-default

Conversation

@eiriktsarpalis

@eiriktsarpaliseiriktsarpalis commented Nov 10, 2021

Copy link
Copy Markdown
Member

Fix#61093.

@eiriktsarpaliseiriktsarpalis added this to the 7.0.0 milestone Nov 10, 2021
@ghost

ghost commented Nov 10, 2021

Copy link
Copy Markdown

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost

ghost commented Nov 10, 2021

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-text-json
See info in area-owners.md if you want to be subscribed.

Issue Details

Fix #61093.

Author:eiriktsarpalis
Assignees:-
Labels:

area-System.Text.Json

Milestone:7.0.0

eiriktsarpalis
eiriktsarpalis commented Nov 10, 2021
View reviewed changes
Comment threadsrc/libraries/System.Text.Json/System.Text.Json.sln
steveharter
steveharter reviewed Nov 11, 2021
View reviewed changes
Comment threadsrc/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs Outdated
/// Gets a read-only, singleton instance of <see cref="JsonSerializerOptions" /> that uses the default configuration.
/// </summary>
/// <remarks>
/// Each <see cref="JsonSerializerOptions" /> instance encapsulates its own converter cache,

@stevehartersteveharterNov 11, 2021

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.

I don't think we need to be specific on "converter cache". Just "caches". It's not just converters, all of the JsonTypeInfo info is cached (properties, constructors, converters).

@eiriktsarpaliseiriktsarpalisNov 15, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Changed, although one could argue that JsonTypeInfo is just implementation detail for object converters. But I agree it should best be kept vague.

steveharter
steveharter approved these changes Nov 11, 2021
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.

Misc comment on doc, otherwise LGTM

deeprobin
deeprobin approved these changes Nov 14, 2021
View reviewed changes

@deeprobindeeprobin 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.

Some style suggestions, but LGTM 😄 👍🏼
Good work!

Comment threadsrc/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/OptionsTests.cs
Comment threadsrc/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/OptionsTests.cs
Comment threadsrc/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/OptionsTests.cs
@eiriktsarpalis
eiriktsarpalisforce-pushed the feature/jsonserializeroptions-default branch from ac06d13 to d0e1b13CompareNovember 15, 2021 09:23
@eiriktsarpalis
eiriktsarpalis merged commit 89d967b into dotnet:mainNov 15, 2021
@eiriktsarpalis
eiriktsarpalis deleted the feature/jsonserializeroptions-default branch November 15, 2021 11:16
This was referenced Nov 29, 2021
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@ghostghost locked as resolved and limited conversation to collaborators Dec 15, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@eerhardteerhardteerhardt left review comments
@stephentoubstephentoubstephentoub approved these changes
@krwqkrwqAwaiting requested review from krwq
@layomialayomiaAwaiting requested review from layomia
+2 more reviewers
@steveharterstevehartersteveharter approved these changes
@deeprobindeeprobindeeprobin approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.Text.Jsonnew-api-needs-documentation

Projects

None yet

Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

[API Proposal]: Expose a JsonSerializerOptions.Default property

5 participants

@eiriktsarpalis@stephentoub@steveharter@eerhardt@deeprobin