[BUG] Set JsonObject current culture to handle float data type properly #13
Conversation
LearnositySDK/Utils/JsonObject.cs
Outdated
| { | ||
| // Set CurrentCulture to proper handle float data type in json | ||
| Thread.CurrentThread.CurrentCulture = CultureInfo.InvariantCulture; | ||
|
|
There was a problem hiding this comment.
@gonzalozawa I'm not a fan of playing with threads. Especially that our SDK may be part of a much bigger environment where changing culture per thread may do a lot of bad things, e.g
- on this thread you're saying that user culture customisations are not taken on account (https://msdn.microsoft.com/en-us/library/system.globalization.cultureinfo.invariantculture(v=vs.110).aspx)
- on all other threads it will be different, which could cause wrong values being saved to DB, or sort of other issues
I would suggest to try this to change sth in this line:
sb.Append(this.df[key].ToString("R"));
Would it work (as per https://msdn.microsoft.com/en-us/library/dwhawy9k.aspx)?
I have limited testing availability here, so even if I accept code review I will assign you back to do functional and take it from there.
|
Hi @karoltarasiuk, thanks for the review! I agree on the comments. I was trying to find a way to set the CultureInfo.InvariantCulture for the JsonObject class and not just for the specific float conversion. |
LearnositySDK/Utils/JsonObject.cs
Outdated
| if (this.di.ContainsKey(key)) | ||
| { | ||
| sb.Append(this.di[key].ToString()); | ||
| sb.Append(this.di[key].ToString("R", CultureInfo.InvariantCulture)); |
There was a problem hiding this comment.
@gonzalozawa why did you do it for integers as well? I think floats should be enough but I may be wrong. Can you give me an example?
|
Hi @karoltarasiuk, updated now. Sorry, my bad on that one, wrong dictionary. |
|
@gonzalozawa all good - pushing to functional now and will assign you. Also don't forget to squash commits before merging. |
Hi @klauste, could you please review? Thanks!