Skip to content

Conversation

@tdykstra
Copy link
Contributor

Related to dotnet/docs#16225

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.

Otherwise, looks good.

@tdykstra tdykstra marked this pull request as ready for review January 3, 2020 23:05
Copy link
Member

@BillWagner BillWagner left a comment

Choose a reason for hiding this comment

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

These look great @tdykstra

I had a couple suggestions that you can choose to add, if you'd like. Then, you can :shipit: when ready.

namespace SystemTextJsonSamples
{
// <SnippetImmutablePoint>
public struct ImmutablePoint
Copy link
Member

Choose a reason for hiding this comment

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

I'd consider making this a readonly struct:

Suggested change
public struct ImmutablePoint
public readonly struct ImmutablePoint

Person value;
Person person;
TypeDiscriminator typeDiscriminator = (TypeDiscriminator)reader.GetInt32();
switch (typeDiscriminator)
Copy link
Member

Choose a reason for hiding this comment

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

This would be a great use for the pattern matching switch expression:

Person person = typeDiscriminator switch
{
    TypeDiscriminator.Customer => new Customer();
    TypeDiscriminator.Employee => new Employee();
    _ => throw new JsonException();
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, except I had to comma-separate the lines instead of terminating them with semicolon. 😄

BTW, this pattern doesn't seem to be in the C# guide switch article , but it is in the C# 8 what's new.

@tdykstra tdykstra merged commit 1fa13d6 into dotnet:master Jan 6, 2020
@tdykstra tdykstra deleted the jnmigrate2 branch January 6, 2020 22:28
@ahsonkhan
Copy link
Contributor

I didn't realize there was a json file checked-in here. Why did it show up as binary?
image

In any case, should we point to the reference of where we got this snippet from (either in the comment within the JSON or as a note in the text before/after it)?

// The JSON data used for the samples was borrowed from https://github.com/Hipo/university-domains-list
// under the MIT License (MIT).

The JSON data used for the samples was borrowed from https://github.com/Hipo/university-domains-list under the MIT License (MIT).

@tdykstra
Copy link
Contributor Author

It's UTF-16 LE, and it only has fictional data.

@ahsonkhan
Copy link
Contributor

it only has fictional data.

Ah gotcha. Didn't realize that.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants