ToUniversalTime() and ToLocalTime() should consider current DST#684
ToUniversalTime() and ToLocalTime() should consider current DST#684udoliess wants to merge 1 commit into
Conversation
|
@udoliess, you're right that there's a problem here (timezone not taken into account), but the solution of going through DateTime is problematic. The whole point of NpgsqlDateTime is to represent value that DateTime can't - DateTime is restricted to years 0-9999, NpgsqlDateTime has a much bigger range. Calling the DateTime property on such out-of-range instances NpgsqlDateTime will throw an exception. In other words, we have to implemented the local/UTC conversions ourselves. Do you want to take a look at how to do this? If not we can open an issue and leave it for now. |
|
Doing the conversion ourselves seems to be problematic to me. Information from tz database (https://en.wikipedia.org/wiki/Tz_database) should to be used. Could we use DateTime.ToUniversalTime(), DateTime.ToLocalTime() for range DateTime.MinValue to DateTime.MaxValue and use TimeZoneInfo.Local.BaseUtcOffset for outside dates? In some cases there would be a leap between year 9999 and 10000 :-( |
|
@udoliess, what you say is certainly possible, but keep in mind that users are supposed to use NpgsqlDateTime precisely because they have dates outside of DateTime.MinValue and DateTime.MaxValue. Another objection is that I think it's preferable to have uniform/coherent ToLocal()/ToUniversal() behavior for NpgsqlDateTime rather than having one behavior for one range (between DateTime.MinValue and DateTime.MaxValue) and another behavior for another range. Imagine the surprise and frustration of a user using NpgqlDateTime with differing values and getting very different behaviors... So in my view either the problem is completely solved for all NpgsqlDateTime values (by using a time zone database as you suggest), or we shouldn't touch this at all and clearly warn users against this... |
|
Note: have added XML docs documenting the issue, when we start publishing the API docs on the website at least it'll give the issue some visibility... |
|
Opened #690 to replace this, closing for now. |
There are some changes in time handling in Npgsql version 3. While looking through I found converts between local and universal time. I think, we should consider more than only the base utc offset. The offset between local time and universal time depends also on daylight saving time. Why not use the DateTime functions?