Skip to content
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.

Sign up for GitHub

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

Jump to bottom

JsonSerializerOptions.Web for JsonSerializerOptions #94370

Merged
merged 5 commits into from
Nov 28, 2023
Merged

JsonSerializerOptions.Web for JsonSerializerOptions #94370

merged 5 commits into from
Nov 28, 2023

Conversation

I-SER-I
Copy link
Contributor

@I-SER-I I-SER-I commented Nov 4, 2023 •
edited
Loading

Close #92181

@dotnet-issue-labeler dotnet-issue-labeler
Copy link

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, 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 added the community-contribution Indicates that the PR has been added by a community member label Nov 4, 2023
@ghost
Copy link

ghost commented Nov 4, 2023

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

Issue Details

null

Author: I-SER-I
Assignees: -
Labels:

area-System.Text.Json, new-api-needs-documentation
Milestone: -

@I-SER-I I-SER-I changed the title feat: add web json serializer option JsonSerializerOptions.Web for JsonSerializerOptions Nov 4, 2023
@I-SER-I I-SER-I changed the title JsonSerializerOptions.Web for JsonSerializerOptions JsonSerializerOptions.Web for JsonSerializerOptions Nov 4, 2023
@I-SER-I
Copy link
Contributor Author

I-SER-I commented Nov 4, 2023

May i change all statement in dontent/runtime new JsonSerializerOptions(JsonSerializerDefaults.Web) to JsonSerializerOptions.Web?

@I-SER-I I-SER-I requested a review from stephentoub November 4, 2023 18:15
@stephentoub
Copy link
Member

May i change all statement in dontent/runtime new JsonSerializerOptions(JsonSerializerDefaults.Web) to JsonSerializerOptions.Web?

Yes, where possible, but most of these cases would involve removing the static readonly field entirely and just changing call sites to use the new property directly.

eiriktsarpalis
eiriktsarpalis reviewed Nov 6, 2023
View reviewed changes
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/OptionsTests.cs Outdated
@@ -1253,7 +1253,7 @@ public static void DefaultSerializerOptions_General()
[Fact]
public static void PredefinedSerializerOptions_Web()
{
var options = new JsonSerializerOptions(JsonSerializerDefaults.Web);
var options = JsonSerializerOptions.Web;
Copy link
Member

Choose a reason for hiding this comment

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

This test is checking the behavior of the constructor and shouldn't be changed. Consider adding a separate test for the singleton instead.

@eiriktsarpalis
Copy link
Member

May i change all statement in dontent/runtime new JsonSerializerOptions(JsonSerializerDefaults.Web) to JsonSerializerOptions.Web?

It depends. Unit tests explicitly there to validate the behavior of the constructor shouldn't be changed, whereas it would be beneficial to update product code that defines its own singletons in lieu of a centrally defined one.

@eiriktsarpalis eiriktsarpalis self-assigned this Nov 15, 2023
@eiriktsarpalis eiriktsarpalis added this to the 9.0.0 milestone Nov 21, 2023
@eiriktsarpalis
Copy link
Member

@I-SER-I would it be possible to address the feedback when possible? Thanks!

@eiriktsarpalis eiriktsarpalis added the needs-author-action An issue or pull request that requires more info or actions from the author. label Nov 21, 2023
@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Nov 27, 2023
This was referenced Nov 27, 2023
Open
Closed
@eiriktsarpalis eiriktsarpalis merged commit f0bd9db into dotnet:main Nov 28, 2023
107 of 109 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@eiriktsarpalis eiriktsarpalis eiriktsarpalis approved these changes

@stephentoub stephentoub Awaiting requested review from stephentoub

Assignees

@eiriktsarpalis eiriktsarpalis

Labels
area-System.Text.Json community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation
Projects
None yet
Milestone
9.0.0
Development

Successfully merging this pull request may close these issues.

[API Proposal]: add JsonSerializerOptions.Web for JsonSerializerOptions
3 participants
@I-SER-I @stephentoub @eiriktsarpalis