Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/timezone support #15

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Feature/timezone support #15

wants to merge 13 commits into from

Conversation

pbogut
Copy link
Owner

@pbogut pbogut commented Sep 2, 2018

Attempt to resolve #14

The idea is to make Timex optional dependency and if available use different date helper to manipulate dates with timezone awareness.

At this point it should respect timezone for dates.

Todo:

  • Add more tests
  • Make sure hour changes are handled correctly

@coveralls
Copy link

coveralls commented Sep 2, 2018

Pull Request Test Coverage Report for Build 75

  • 46 of 46 (100.0%) changed or added relevant lines in 8 files are covered.
  • 15 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-5.08%) to 94.915%

Files with Coverage Reduction New Missed Lines %
lib/recurring_events/date.ex 15 83.15%
Totals Coverage Status
Change from base Build 62: -5.08%
Covered Lines: 280
Relevant Lines: 295

💛 - Coveralls

@pbogut
Copy link
Owner Author

pbogut commented Sep 2, 2018

So if the rule is :hourly, :minutely etc. module is using Timex.shift, so it should handle timezone correctly.

Now, I'm not sure what to do with by_hourif it goes into timezone change. Two examples:

Let say we are working with Europe/Warsaw timezone. When we have by_hour: 2 rule and time is changed form summer time (GTM+2) to winter time (GTM+1). We can have 2 AM GTM+1 and 2 AM GTM+2 as we are moving clock back from 3am to 2am. How to decide which one should be used?

In reverse example we would change clock from 1:59am to 3:00am, so there is no 2am at all. Should 3am be set instead?

@bgentry
Copy link

bgentry commented Sep 2, 2018

Let say we are working with Europe/Warsaw timezone. When we have by_hour: 2 rule and time is changed form summer time (GTM+2) to winter time (GTM+1). We can have 2 AM GTM+1 and 2 AM GTM+2 as we are moving clock back from 3am to 2am. How to decide which one should be used?

In reverse example we would change clock from 1:59am to 3:00am, so there is no 2am at all. Should 3am be set instead?

I submitted #16 with broken tests for both of these cases. The expected behavior is outlined by RFC5545:

If, based on the definition of the referenced time zone, the local
time described occurs more than once (when changing from daylight
to standard time), the DATE-TIME value refers to the first
occurrence of the referenced time. Thus, TZID=America/
New_York:20071104T013000 indicates November 4, 2007 at 1:30 A.M.
EDT (UTC-04:00). If the local time described does not occur (when
changing from standard to daylight time), the DATE-TIME value is
interpreted using the UTC offset before the gap in local times.
Thus, TZID=America/New_York:20070311T023000 indicates March 11,
2007 at 3:30 A.M. EDT (UTC-04:00), one hour after 1:30 A.M. EST
(UTC-05:00).

@bgentry
Copy link

bgentry commented Sep 3, 2018

@pbogut I just want to make sure you saw the following note from #16 about one of those broken tests needing some changes:

Note that the expected value here is also wrong because it has an ambiguous timezone result (since 1:30AM occurs twice). We likely need to explicitly choose the expected timezone for this result to make sure it is the UTC-7 / PDT value, not the UTC-8 PST value (or ambiguous).

@pbogut
Copy link
Owner Author

pbogut commented Sep 4, 2018

@bgentry yes, I've read the comment, thank you for that. I will take it into account and make sure tests are checking correct timezone as well.

@voughtdq
Copy link
Contributor

voughtdq commented Nov 27, 2018

So, I think we can go off this:

When using Timex, Timex.to_datetime({{2007, 11, 4}, {1, 30, 0}}, "America/New_York") will return an AmbiguousDateTime struct. Because RFC5545 says to refer to the first occurrence, we can safely select the :before field in the struct. When the time is impossible (like 2007-3-11 2:30AM America/New_York) we get an {:error, _} tuple. In all other cases, we should get a %DateTime{} with the correct tz ref.

Here's an example of what I'm thinking about:

dt = {{2017, 11, 4}, {1, 30, 0}}
tz = "America/New_York"
 case Timex.to_datetime(dt, tz) do
   %DateTime{} = datetime ->
     datetime
   %Timex.AmbiguousDateTime{before: before, after: _after} ->
     # ambiguous, use the first occurrence per the spec
     before
   {:error, {:could_not_resolve_timezone, ^tz, _gregorian_seconds, :wall}} ->
     # non-existent time, add 1 hour
     {date, {h, m, s}} = dt
     Timex.to_datetime({date, {h + 1, m, s}}, tz)
 end

Now the last case with the {:error, _} tuple may need to be reworked a little bit because this assumes the only resolution error would be due to daylight savings and that all DST offsets are always 1 hour. (and the possibility that h+1 ends up being something like 24)

@pbogut
Copy link
Owner Author

pbogut commented Nov 27, 2018

@voughtdq that's very helpful. I did implementation for dates but couldn't figure out good way to do time. I think your idea should work all right. Will test it and see how its doing. Thanks.

@tlvenn
Copy link

tlvenn commented Jul 30, 2019

@pbogut have you been able to make some progress on this ?

@pbogut
Copy link
Owner Author

pbogut commented Aug 11, 2019

I didn't spent too much time on this I'm afraid, I started it but its still in progress.
I'm happy to accept PR if someone is able to do that, I may not have time to look at it for another few months unfortunately.

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.

library does not respect DST boundaries (no timezone awareness)
5 participants