Show default behavior of reader/writer in DateTime customization samples #1412
Conversation
|
I just noticed that this standard folder doesn't have a language identification which will make harder to enable CI tests later for this folder, if we wanted. Also, there's no obvious path if we want to add a VB counterpart. Shouldn't this be added to the https://github.com/dotnet/samples/tree/master/snippets/csharp folder instead @rpetrusha @BillWagner? |
|
I think the basic path is fine -- I don't think we want to primarily segregate code based on language, so that the corresponding examples in specific languages have completely different paths, But we do want to differentiate the language in the event that we have examples in multiple languages. So it seems to me that a good path is snippets/standard/datetime/json/custom-reading-with-utf8jsonreader/cs, as you suggested in the first paragraph of your comment, @mairaw. |
|
That also means that we should move the existing examples to a cs directory. |
rpetrusha
left a comment
rpetrusha
left a comment
There was a problem hiding this comment.
I left one comment for you to consider -- I was surprised by the use of a discard.
| { | ||
| try | ||
| { | ||
| DateTimeOffset _ = json.GetDateTimeOffset(); |
There was a problem hiding this comment.
This seems contrived, and the intent is unclear here: why would anyone ever use a discard in this situation, except to ignore a success and handle a failure?
There was a problem hiding this comment.
I sort of agree. I would either use a separate sample to show-case the error case (and not in a try/catch), or just call that out in the doc itself rather than adding it to the existing sample for customization.
There was a problem hiding this comment.
Also, looking at https://docs.microsoft.com/en-us/dotnet/standard/datetime/system-text-json-support, you already have a sample showing what happens when using the reader.GetDateTime on an unsupported type.
In that case, this change isn't necessary.
|
Sounds good @rpetrusha. We don't currently test the snippets folder anyway. It seems we use csharp as the folder name whenever we have C# samples though. |
| writer.WriteString("date", dateStr); | ||
| writer.WritePropertyName("dateFormats"); | ||
| writer.WriteStartArray(); | ||
| writer.WriteStringValue(utcNow); |
There was a problem hiding this comment.
Hmm, re-reading the current doc again, looks like we already show-cased the default reader/writer behavior with a sample up-front. I am on the fence of this change on the writer and would be OK with just leaving things as is (or showing both formats explicitly). When reviewing previously I hadn't realized those cases were already covered int he doc.
So, we could just close this PR and revert dotnet/docs@5829ca4.
|
I'm closing this PR because the target document for these samples already showcases the default behavior. This change is redundant: #1412 (comment). I'll open another PR adding a parent "cs" folder to the paths of all the samples used in the Json-DateTime doc: #1412 (comment). |
4 participants
Addresses dotnet/docs#14056 (comment).