search

mathnet / mathnet-spatial

Math.NET Spatial
http://spatial.mathdotnet.com
MIT License
376 stars 132 forks source link

Update XmlExt.cs #179

Closed rsinnig closed 4 years ago

rsinnig commented 4 years ago

line 190: writer.WriteValue(value.ToString("G15")); ToString() without culture format provider parameter uses local culture. This causing issues when deserializing on machines with comma decimal separator.

JohanLarsson commented 4 years ago

Ugh, this was really bad and in so many ways. This should not have been public and lack of InvariantCulture is embarrassing.

Thanks for this PR. This is a breaking change.

JohanLarsson commented 4 years ago

XmlExt is [Obsolete] https://github.com/mathnet/mathnet-spatial/blob/379013b8280aca8a45147e3612fa6bd2e59f5519/src/Spatial/Obsolete/XmlExt.cs#L14

Not sure we should do a breaking change on an obsolete type.

rsinnig commented 4 years ago

Sorry forgot to insert the using System.Globalization;

Actually I don't understand why it's marked obsolete but still in use.

But I had to modify this line on my local copy of code to get deserialization working.

Writing double values with comma decimal separator into XML serialisation is definitely not correct, since deserialization assumes culture neutral formatting by default.

JohanLarsson commented 4 years ago

Actually I don't understand why it's marked obsolete but still in use.

Are there still usages of this in the library?

Writing double values with comma decimal separator into XML serialisation is definitely not correct, since deserialization assumes culture neutral formatting by default.

Agreed, this was an unfortunate mistake.

rsinnig commented 4 years ago

IXmlSerializable.WriteXml(XmlWriter writer) explicit implementation of Point2D, Point3D, UnitVector3D, Vector2D, Vector3D and Angle seems to use this extension method.

rsinnig commented 4 years ago

Maybe I should cancel the pull request. The actual issue seems to be, that mentioned classes Point3D, … are using the obsolete methods instead of methods inside MathNet.Spatial.Internals.XmlWriterExt class. I will try to fix it by using MathNet.Spatial.Internals.XmlWriterExt.WriteElement(this XmlWriter writer, string name, double value, string format) But I can't do that work in this week.

rsinnig commented 4 years ago

Cancelling of request to be done from my side? It's my first PR on GitHub. I'm not experienced, with the usual process.

JohanLarsson commented 4 years ago

Yes I don't think fixing this in the obsolete method is ideal so closing this pull request is probably good. Thank you for taking the time to contribute an d what you found is very troubling.

JohanLarsson commented 4 years ago

Cancelling of request to be done from my side?

I closed it, but could have been done from either side.