Make ParseDateTimeTimeZoneSafe culture aware #58
Conversation
Fixes issue https://github.com/sinairv/YAXLib/issues/35 As @gitgordon pointed out, the public static method ParseDateTimeTimeZoneSafe does not use the argument IFormatProvider, and therefore the culture which DateTimeOffset.TryParse is expected to use is not set by YAXLib code. So depending on the culture previously set (e.g. system default culture) it might work or fail.
Codecov Report
@@ Coverage Diff @@
## master #58 +/- ##
=========================================
Coverage ? 69.37%
=========================================
Files ? 13
Lines ? 2622
Branches ? 634
=========================================
Hits ? 1819
Misses ? 621
Partials ? 182
Continue to review full report at Codecov.
|
|
👍 |
|
Would be great if you add an unit test :) |
|
Hm, you mean mimic the bug? Doesn't really make sense I'm afraid :) |
|
Can't agree on this. This could be a serious bug for a lot of non-english users. That the unit test was passing already, shows only that it isn't a good test. |
|
Looks like I'm outvoted :) |
|
Great! |
YAXLibTests/SerializationTest.cs
Outdated
|
|
||
| Assert.That(expected, Is.EqualTo(usResult), "Checking US is as expected!"); | ||
| // deserialization results | ||
| Assert.AreEqual(0, faDesResult?.CompareTo(frDesResult), "Comparing deserialized FA and FR"); |
There was a problem hiding this comment.
Why not Assert.AreEqual(faDesResult,frDesResult) ?
There was a problem hiding this comment.
Strange enough, but this fails...
There was a problem hiding this comment.
both are strings right?
There was a problem hiding this comment.
No, a CultureSample instance.
There was a problem hiding this comment.
That's why I introduced the IComparable to CultureSample.
There was a problem hiding this comment.
Ah then I would be better to generate equals(). Resharper can do that for you :)
After this comment I was excepting to test it just with other current cultures? e.g.
Which could be done with Unit test parameters |
|
Thanks for the fix. Great work. |
|
yes great work @axunonb ! 🥂 |
Fixes issue https://github.com/sinairv/YAXLib/issues/35
As @gitgordon pointed out, the public static method
ParseDateTimeTimeZoneSafedoes not use the argumentIFormatProvider, and so the culture whichDateTimeOffset.TryParseis expected to use is not set by YAXLib code. Depending on the system culture or the culture set by any code for the current thread it might work or fail.With NUnit3 tests the current culture was set
System.Globalization.CultureInfo.InvariantCulturewhich was the reason for the tests to succeed.This fix seems to be kind of urgent IMHO.