Skip to content

Conversation

@carlossanlop
Copy link
Contributor

New batch.

@carlossanlop carlossanlop added new-content Indicates PRs that contain new articles waiting-on-reviews Indicates PRs that cannot be merged because of the lack of reviews 🏁 Release: .NET Core 3.0 :checkered_flag: Release: .NET Core 3.0 labels Jul 31, 2019
@carlossanlop carlossanlop added this to the July 2019 milestone Jul 31, 2019
@carlossanlop carlossanlop self-assigned this Jul 31, 2019
@mairaw mairaw modified the milestones: July 2019, August 2019 Jul 31, 2019
@rpetrusha rpetrusha removed the waiting-on-reviews Indicates PRs that cannot be merged because of the lack of reviews label Jul 31, 2019
Copy link

@rpetrusha rpetrusha left a comment

Choose a reason for hiding this comment

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

I've made a number of changes, if you'd like to review them, @carlossanlop. I'll merge when the build completes successfully.

Exception conditions were appearing multiple times in the file. Was there a change to your porting tool that might have caused this?

@carlossanlop
Copy link
Contributor Author

carlossanlop commented Jul 31, 2019

Exception conditions were appearing multiple times in the file. Was there a change to your porting tool that might have caused this?

@rpetrusha I doubt it, unless you're seeing the same exceptions in all the PRs I submitted for Json yesterday.

What was the error? Maybe it was an intermittent issue in the Docs build?

Edit: I see what you mean. I thought you meant an exception thrown during the build process. Let me check the code.

Copy link
Member

@jozkee jozkee left a comment

Choose a reason for hiding this comment

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

Left some comments.

@carlossanlop
Copy link
Contributor Author

@rpetrusha It's not my tool's fault, I ran it again and all the exceptions were added only once.
It was my own fault when I was fixing the exception messages. They all started with "Thrown when" and used Control+F in VS to remove the additional text. But apparently I did something wrong and also duplicated them all 5 times. Sorry about that. If I find any others, I'll remove them.
CC @jozkee who also reported the problem.

@carlossanlop carlossanlop requested a review from jozkee August 1, 2019 20:59
@carlossanlop carlossanlop added the changes-addressed Indicates PRs that had all comments addressed and are awaiting for new review label Aug 1, 2019
@carlossanlop carlossanlop requested a review from rpetrusha August 1, 2019 21:00
@carlossanlop
Copy link
Contributor Author

I addressed the comments and fixed some missing values. The only thing blocking this from getting merged is the question to @ahsonkhan

@mairaw mairaw added the waiting-on-feedback Indicates PRs that are waiting for feedback from SMEs before they can be merged label Aug 6, 2019
Co-Authored-By: Ahson Khan <[email protected]>
@carlossanlop
Copy link
Contributor Author

@rpetrusha the last comment in this PR has been addressed. If the build passes, I think we can get this merged.

@carlossanlop carlossanlop removed the changes-addressed Indicates PRs that had all comments addressed and are awaiting for new review label Aug 6, 2019
@carlossanlop carlossanlop added verify-build-before-merge and removed waiting-on-feedback Indicates PRs that are waiting for feedback from SMEs before they can be merged labels Aug 6, 2019
Copy link
Contributor

@ahsonkhan ahsonkhan left a comment

Choose a reason for hiding this comment

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

Partially reviewed. Left some feedback.

Co-Authored-By: Ahson Khan <[email protected]>
@rpetrusha
Copy link

I'll merge this PR now, @carlossanlop.

@rpetrusha rpetrusha merged commit ec7b039 into dotnet:master Aug 7, 2019
@carlossanlop carlossanlop deleted the JsonSerializerOptions branch August 7, 2019 18:40
This property can be set to <xref:System.Text.Json.JsonNamingPolicy.CamelCase?displayProperty=nameWithType> to specify a camel-casing policy.
This property can be set to <xref:System.Text.Json.JsonNamingPolicy.CamelCase> to specify a camel-casing policy.
Copy link
Contributor

Choose a reason for hiding this comment

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

This remark got duplicated with the latest change.

## Remarks
Once serialization or deserialization occurs, the list cannot be modified.
Copy link
Contributor

Choose a reason for hiding this comment

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

It should state, "should not be modified", not cannot. There is nothing preventing the caller from modifying the list. The changes just won't be reflected when (de)serializing.

cc @steveharter

Copy link
Contributor

Choose a reason for hiding this comment

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

The caller cannot modify the list. They will get an InvalidOperationException.

Copy link
Contributor

@ahsonkhan ahsonkhan Aug 8, 2019

Choose a reason for hiding this comment

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

Really? Interesting. We should document the exception then, no? Similar to other properties:
https://docs.microsoft.com/en-us/dotnet/api/system.text.json.jsonserializeroptions.allowtrailingcommas?view=netcore-3.0#exceptions

Verified, yep.

[Fact]
public static void ConverterWithCallback()
{
    const string json = @"{""Name"":""MyName""}";

    var options = new JsonSerializerOptions();
    options.Converters.Add(new CustomerCallbackConverter());

    Customer customer = JsonSerializer.Deserialize<Customer>(json, options);
    Assert.Equal("MyNameHello!", customer.Name);

    // Throws System.InvalidOperationException : Serializer options cannot be changed once serialization or deserialization has occurred.
    options.Converters.Add(new CustomerCallbackConverter());
}

<summary>To be added.</summary>
<value>To be added.</value>
<summary>Gets or sets a value that determines whether <see langword="null" /> values are ignored during serialization and deserialization. The default value is <see langword="false" />.</summary>
<value><see langword="true" /> to ignore null values during serialization and deserialization; otherwise, see langword="false" />.</value>
Copy link
Contributor

Choose a reason for hiding this comment

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

This is missing a start tag

Suggested change
<value><see langword="true" /> to ignore null values during serialization and deserialization; otherwise, see langword="false" />.</value>
<value><see langword="true" /> to ignore null values during serialization and deserialization; otherwise, <see langword="false" />.</value>

<Docs>
<summary>To be added.</summary>
<value>To be added.</value>
<summary>Gets or sets a value that determines whether <see langword="null" /> values are ignored during serialization and deserialization. The default value is <see langword="false" />.</summary>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have "the default value" specified in the summary text in some cases, but in remarks in others. Worth being consistent and moving them all to the remarks?

## Remarks
There is a performance cost associated with case-insensitie comparison (that is, when `PropertyNameCaseInsensitive` is `true`).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
There is a performance cost associated with case-insensitie comparison (that is, when `PropertyNameCaseInsensitive` is `true`).
There is a performance cost associated with case-insensitive comparison (that is, when `PropertyNameCaseInsensitive` is `true`).

<value>To be added.</value>
<remarks>To be added.</remarks>
<summary>Gets or sets a value that specifies the policy used to convert a property's name on an object to another format, such as camel-casing. </summary>
<value>One of the enum values from <see cref="T:System.Text.Json.JsonNamingPolicy" />.</value>
Copy link
Contributor

@ahsonkhan ahsonkhan Aug 8, 2019

Choose a reason for hiding this comment

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

JsonNamingPolicy is not an enum.

Suggested change
<value>One of the enum values from <see cref="T:System.Text.Json.JsonNamingPolicy" />.</value>
<value>An instance of a <see cref="T:System.Text.Json.JsonNamingPolicy" />.</value>

<value>To be added.</value>
<remarks>To be added.</remarks>
<summary>Gets or sets a value that defines how comments are handled during deserialization.</summary>
<value>A value that indicates whether comments are allowed, disallowed, or skipped.</value>
Copy link
Contributor

@ahsonkhan ahsonkhan Aug 8, 2019

Choose a reason for hiding this comment

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

The enum value JsonCommentHandling.Allow isn't supported here.

Suggested change
<value>A value that indicates whether comments are allowed, disallowed, or skipped.</value>
<value>A value that indicates whether comments are disallowed or ignored.</value>

<summary>To be added.</summary>
<value>To be added.</value>
<remarks>To be added.</remarks>
<summary>Gets or sets a value that defines whether JSON should use pretty printing. By default, JSON is serialized without any extra white space.</summary>
Copy link
Contributor

Choose a reason for hiding this comment

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

"JSON using pretty printing" doesn't make sense.

Suggested change
<summary>Gets or sets a value that defines whether JSON should use pretty printing. By default, JSON is serialized without any extra white space.</summary>
<summary>Gets or sets a value that defines whether JSON should be written pretty printed. By default, JSON is serialized without any extra white space.</summary>

Pretty printing includes:
- Indenting nested JSON tokens.
- Adding new lines
Copy link
Contributor

@ahsonkhan ahsonkhan Aug 8, 2019

Choose a reason for hiding this comment

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

Suggested change
- Adding new lines
- Adding new lines between between consecutive JSON object or array elements.

Copy link
Contributor

@ahsonkhan ahsonkhan left a comment

Choose a reason for hiding this comment

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

Left some correctness feedback/typos.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🏁 Release: .NET Core 3.0 :checkered_flag: Release: .NET Core 3.0 new-content Indicates PRs that contain new articles

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants