-
-
Notifications
You must be signed in to change notification settings - Fork 469
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.
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
Added the deserialize fix and test for the UTC DateTime #613
Conversation
value = DateTime.Parse(scalar.Value, CultureInfo.InvariantCulture); | ||
var style = scalar.Value.EndsWith("Z") | ||
? DateTimeStyles.AdjustToUniversal | DateTimeStyles.AssumeUniversal | ||
: scalar.Value.Length > 27 ? DateTimeStyles.AssumeLocal : DateTimeStyles.None; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing the length seems a bit fragile. Maybe a regex would be more robust?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have the skills to create a good regex for this, so I don't know if it would be more robust. The previous implementation is still the same, DateTimeStyles.None
is the default. And the 'o' roundtrip is compliant with the ISO 8601, so I thought this won't break other implementations but would only add the correct TimeZone if this is known.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know you are a busy man at the moment but is there a chance we can resolve this? What kind of regex are you thinking of?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry, I haven't had much free time. I made some experiments and it seems that instead of forcing the adjustment, you can simply pass the DateTimeStyles.RoundtripKind
value to the style and you will get the correct kind. Have you tried that instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah.. I've overlooked that one, that is definitely a better solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can always use the DateTimeStyles.RoundtripKind
, without any test:
value = DateTime.Parse(scalar.Value, CultureInfo.InvariantCulture, DateTimeStyles.RoundtripKind);
Thanks for this and sorry about the slow response time. |
No problem. I’m glad it’s fixed. |
One question, when do you think this will be released to the latest nuget? |
This feature has been released in version 11.2.0. |
Deserialize the datetime to correct timezone as described on the Microsoft documentation for the "o" Roundtrip
https://docs.microsoft.com/en-us/dotnet/standard/base-types/standard-date-and-time-format-strings#Roundtrip