CSHARP-3268: Investigate Military Time Failures #1687
Conversation
| @@ -1373,7 +1373,7 @@ private DateTime ParseJavaScriptDateTimeString(string dateTimeString) | |||
| { | |||
| // if DateTime.TryParse succeeds we're done, otherwise assume it's an RFC 822 formatted DateTime string | |||
| DateTime dateTime; | |||
| if (DateTime.TryParse(dateTimeString, out dateTime)) | |||
| if (!dateTimeString.EndsWith("A") && DateTime.TryParse(dateTimeString, out dateTime)) | |||
There was a problem hiding this comment.
For some reason DateTime.TryParse returns true for time zone A but who knows what it thinks the A means.
If the string ends in A then we should use our own parsing method.
There was a problem hiding this comment.
Any chance there might be a collision with some other format ending with "A"?
Also there is not handling for "Z", is it a known abbreviation for UTC?
There was a problem hiding this comment.
In the existing test data nothing else ends with "A". But you are right, there might be a timezone abbreviation we don't know about that ends in A.
We could use a regular expression:
if (!Regex.Matches(dateString, "[0-9 ]A$") && ...)
There was a problem hiding this comment.
We have a test case for Z. I think it is a standard abbreviation for UTC and is handled by DateTime.TryParse.
There was a problem hiding this comment.
Thanks, just wanted to confirm that Z is for UTC.
Looking at .net code, seems that A is related to Era (AD is also valid).
What worries me is that "Mon, 10 Oct 2011 11:22:33 A" is parsed to a certain value in .NET, and we'll be overriding that. If going with this change we need to convince ourselves that this is the right thing to do.
There was a problem hiding this comment.
Also how will this "Mon, 10 Oct 2011 11:22:33 AD" get parsed?
| case "W": offset = TimeSpan.FromHours(10); break; | ||
| case "X": offset = TimeSpan.FromHours(11); break; | ||
| case "Y": offset = TimeSpan.FromHours(12); break; | ||
| case "A": offset = TimeSpan.FromHours(1); break; |
There was a problem hiding this comment.
The sign of all the offsets for A to Y was backwards.
|
|
||
| private Test[] _tests = new Test[] | ||
| { | ||
| public static object[][] TestParseDatesData => [ |
There was a problem hiding this comment.
Convert to supply data to a Theory.
| @@ -1373,7 +1373,7 @@ private DateTime ParseJavaScriptDateTimeString(string dateTimeString) | |||
| { | |||
| // if DateTime.TryParse succeeds we're done, otherwise assume it's an RFC 822 formatted DateTime string | |||
| DateTime dateTime; | |||
| if (DateTime.TryParse(dateTimeString, out dateTime)) | |||
| if (!dateTimeString.EndsWith("A") && DateTime.TryParse(dateTimeString, out dateTime)) | |||
There was a problem hiding this comment.
Any chance there might be a collision with some other format ending with "A"?
Also there is not handling for "Z", is it a known abbreviation for UTC?
| @@ -1373,7 +1373,7 @@ private DateTime ParseJavaScriptDateTimeString(string dateTimeString) | |||
| { | |||
| // if DateTime.TryParse succeeds we're done, otherwise assume it's an RFC 822 formatted DateTime string | |||
| DateTime dateTime; | |||
| if (DateTime.TryParse(dateTimeString, out dateTime)) | |||
| if (!dateTimeString.EndsWith("A") && DateTime.TryParse(dateTimeString, out dateTime)) | |||
There was a problem hiding this comment.
In the existing test data nothing else ends with "A". But you are right, there might be a timezone abbreviation we don't know about that ends in A.
We could use a regular expression:
if (!Regex.Matches(dateString, "[0-9 ]A$") && ...)
| @@ -1373,7 +1373,7 @@ private DateTime ParseJavaScriptDateTimeString(string dateTimeString) | |||
| { | |||
| // if DateTime.TryParse succeeds we're done, otherwise assume it's an RFC 822 formatted DateTime string | |||
| DateTime dateTime; | |||
| if (DateTime.TryParse(dateTimeString, out dateTime)) | |||
| if (!dateTimeString.EndsWith("A") && DateTime.TryParse(dateTimeString, out dateTime)) | |||
There was a problem hiding this comment.
We have a test case for Z. I think it is a standard abbreviation for UTC and is handled by DateTime.TryParse.
| case "V": offset = TimeSpan.FromHours(-9); break; | ||
| case "W": offset = TimeSpan.FromHours(-10); break; | ||
| case "X": offset = TimeSpan.FromHours(-11); break; | ||
| case "Y": offset = TimeSpan.FromHours(-12); break; |
There was a problem hiding this comment.
Did you mean that we should add a case for "Z" here?
I don't see how we would ever reach here with a "Z" because DateTime.TryParse would have already handled it.
| @@ -1373,7 +1373,7 @@ private DateTime ParseJavaScriptDateTimeString(string dateTimeString) | |||
| { | |||
| // if DateTime.TryParse succeeds we're done, otherwise assume it's an RFC 822 formatted DateTime string | |||
| DateTime dateTime; | |||
| if (DateTime.TryParse(dateTimeString, out dateTime)) | |||
| if (!dateTimeString.EndsWith("A") && DateTime.TryParse(dateTimeString, out dateTime)) | |||
There was a problem hiding this comment.
Also how will this "Mon, 10 Oct 2011 11:22:33 AD" get parsed?
No description provided.