Skip to content

ToUniversalTime() and ToLocalTime() should consider current DST#684

Closed
udoliess wants to merge 1 commit into
npgsql:developfrom
udoliess:npgsql-develop1
Closed

ToUniversalTime() and ToLocalTime() should consider current DST#684
udoliess wants to merge 1 commit into
npgsql:developfrom
udoliess:npgsql-develop1

Conversation

@udoliess
Copy link
Copy Markdown
Contributor

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?

@roji
Copy link
Copy Markdown
Member

roji commented Jul 30, 2015

@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.

@udoliess
Copy link
Copy Markdown
Contributor Author

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 :-(

@roji
Copy link
Copy Markdown
Member

roji commented Jul 31, 2015

@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...

roji added a commit that referenced this pull request Aug 7, 2015
roji added a commit that referenced this pull request Aug 7, 2015
Relates to #684

(cherry picked from commit d3d9640)
@roji
Copy link
Copy Markdown
Member

roji commented Aug 7, 2015

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...

@roji
Copy link
Copy Markdown
Member

roji commented Aug 9, 2015

Opened #690 to replace this, closing for now.

@roji roji closed this Aug 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants