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

Daylight Savings not handled properly #147

Open
bricker opened this issue Oct 28, 2012 · 10 comments · May be fixed by #396
Open

Daylight Savings not handled properly #147

bricker opened this issue Oct 28, 2012 · 10 comments · May be fixed by #396
Labels

Comments

@bricker
Copy link

bricker commented Oct 28, 2012

Daylight savings "falls back" in the states on November 4th, 2012, at exactly 2am. It goes backwards and repeats the 1am hour.

Time.new(2012, 11, 4, 0)
# => 2012-11-04 00:00:00 -0700

Chronic.parse("2012-11-04 at 12am")
# => 2012-11-04 01:00:00 -0700

Chronic.parse("2012-11-04")
# => 2012-11-04 11:00:00 -0800 

Those first two dates should return the same, and I would expect the last one to return 12pm. Hopefully this is a silly mistake I'm making, or there is a reasonable explanation. DST is a nightmare!

@travisbell
Copy link

Probably related, this morning, my Chronic calls started straight up failing right at 2AM when DST kicked over.

Chronic.parse('tomorrow at 4:00 pm')

Is now nil. Argh?

1.9.2p320 :006 > Chronic.parse('tomorrow at 4:00 pm')
 => nil 
1.9.2p320 :007 > 

I'm currently setting the timezone like so, if that matters:

Time.zone = "Pacific Time (US & Canada)"
Chronic.time_class = Time.zone

Setting the timezone to 'UTC' seems to fix it. But I am wondering why this worked all the way until yesterday but not today?

@markerdmann
Copy link

I ran into the exact same issue today described by @travisbell above.

@wakiki
Copy link

wakiki commented Mar 12, 2013

I also encountered the same error as @bricker - has there been any updates on this?

@leejarvis
Copy link
Collaborator

No there hasn't, I'm struggling to find time for Chronic at the moment. Hoping to start hitting some of these as soon as possible

@markerdmann
Copy link

@injekt I have some time to work on this. I'll fork and send a pull request if I have any luck. Do you have an idea of where in the codebase I might start looking to find the source of the bug?

@wakiki
Copy link

wakiki commented Mar 12, 2013

I don't know if this is the same issue but I noted the following in issue #179

1.9.3p362 :021 > Time.current
=> Tue, 12 Mar 2013 11:01:30 GMT +00:00
1.9.3p362 :017 > Time.zone = "London"
=> "London"
1.9.3p362 :018 > Chronic.time_class = Time.zone
=> (GMT+00:00) London
1.9.3p362 :019 > Chronic.parse("April 1st at 9am")
=> Mon, 01 Apr 2013 09:00:00 BST +01:00
1.9.3p362 :020 > Chronic.parse("20 days from now at 9am")
=> Mon, 01 Apr 2013 10:00:00 BST +01:00

Interestingly, specifying "April 1st" works fine whereas "20 days from now" doesn't work.

Chronic 0.9.1
Rails 3.2.6

@leejarvis
Copy link
Collaborator

@markerdmann sorry I hadn't noticed your message until today. I can't reproduce the original issue so it'll probably take me a little longer to fix. Usually I would enable debug mode (Chronic.debug = true) then jump into the method that handles this sequence (in Handlers) then debug things from there. It's not always a quick process and there's a few subtle bugs in Chronic that aren't easy to fix. I do still have a rewrite that's currently in progress but again I'm finding it hard to find work on this at the moment, everything is tied up with work

@stanhu
Copy link

stanhu commented Nov 22, 2019

We ran into this in floraison/et-orbi#23 as well. I think these lines are questionable:

yesterday_midnight = midnight - full_day
tomorrow_midnight = midnight + full_day
offset_fix = midnight.gmt_offset - tomorrow_midnight.gmt_offset
tomorrow_midnight += offset_fix
catch :done do
if pointer == :future
if @type.ambiguous?
[midnight + @type.time + offset_fix, midnight + half_day + @type.time + offset_fix, tomorrow_midnight + @type.time].each do |t|
(@current_time = t; throw :done) if t >= @now
end
else
[midnight + @type.time + offset_fix, tomorrow_midnight + @type.time].each do |t|
(@current_time = t; throw :done) if t >= @now
end

Try passing in Chronic.parse('2020-11-01 00:00:00') on a system that has daylight savings:

Time.parse('2020-11-01 00:00:00')
=> 2020-11-01 00:00:00 -0700
Chronic.parse('2020-11-01 00:00:00')
=> 2020-11-01 01:00:00 -0700

Why did Chronic advance an hour like that?

When a date like this is passed in Chronic.parse, offset_fix becomes 3600 (1 hour), and the starting time advances by an hour.

If I try to get rid of this offset_fix:

diff --git a/lib/chronic/repeaters/repeater_time.rb b/lib/chronic/repeaters/repeater_time.rb
index 0a496e7..208553e 100644
--- a/lib/chronic/repeaters/repeater_time.rb
+++ b/lib/chronic/repeaters/repeater_time.rb
@@ -87,21 +87,21 @@ module Chronic
         catch :done do
           if pointer == :future
             if @type.ambiguous?
-              [midnight + @type.time + offset_fix, midnight + half_day + @type.time + offset_fix, tomorrow_midnight + @type.time].each do |t|
+              [midnight + @type.time, midnight + half_day + @type.time + offset_fix, tomorrow_midnight + @type.time].each do |t|
                 (@current_time = t; throw :done) if t >= @now
               end
             else
-              [midnight + @type.time + offset_fix, tomorrow_midnight + @type.time].each do |t|
+              [midnight + @type.time, tomorrow_midnight + @type.time].each do |t|
                 (@current_time = t; throw :done) if t >= @now
               end
             end
           else # pointer == :past
             if @type.ambiguous?
-              [midnight + half_day + @type.time + offset_fix, midnight + @type.time + offset_fix, yesterday_midnight + @type.time + half_day].each do |t|
+              [midnight + half_day + @type.time, midnight + @type.time + offset_fix, yesterday_midnight + @type.time + half_day].each do |t|
                 (@current_time = t; throw :done) if t <= @now
               end
             else
-              [midnight + @type.time + offset_fix, yesterday_midnight + @type.time].each do |t|
+              [midnight + @type.time, yesterday_midnight + @type.time].each do |t|
                 (@current_time = t; throw :done) if t <= @now
               end
             end

Tests like the following fail:

# ambiguous - resolve to this morning
t = Chronic::RepeaterTime.new('900')
t.start = @begin_daylight_savings
assert_equal Time.local(2008, 3, 9, 9), t.next(:future).begin
.

stanhu added a commit to stanhu/chronic that referenced this issue Nov 23, 2019
Previously the offset change for Daylight Savings was unnecessarily
added all the time, resulting in this discrepancy when the local time
was set to UTC-7:

```
Time.parse('2020-11-01 00:00:00')
=> 2020-11-01 00:00:00 -0700
Chronic.parse('2020-11-01 00:00:00')
=> 2020-11-01 01:00:00 -0700
```

Each time we add an offset to a base time, we need to check whether this
crosses Daylight Savings boundaries. Only if it does should we adjust
the time by the offset.

Closes mojombo#147
stanhu added a commit to stanhu/chronic that referenced this issue Nov 23, 2019
Previously the offset change for Daylight Savings was unnecessarily
added all the time, resulting in this discrepancy when the local time
was set to UTC-7:

```
Time.parse('2020-11-01 00:00:00')
=> 2020-11-01 00:00:00 -0700
Chronic.parse('2020-11-01 00:00:00')
=> 2020-11-01 01:00:00 -0700
```

Each time we add an offset to a base time, we need to check whether this
crosses Daylight Savings boundaries. Only if it does should we adjust
the time by the offset.

Closes mojombo#147
@stanhu stanhu linked a pull request Nov 23, 2019 that will close this issue
stanhu added a commit to stanhu/chronic that referenced this issue Nov 23, 2019
Previously the offset change for Daylight Savings was unnecessarily
added all the time, resulting in this discrepancy when the local time
was set to UTC-7:

```
Time.parse('2020-11-01 00:00:00')
=> 2020-11-01 00:00:00 -0700
Chronic.parse('2020-11-01 00:00:00')
=> 2020-11-01 01:00:00 -0700
```

Each time we add an offset to a base time, we need to check whether this
crosses Daylight Savings boundaries. Only if it does should we adjust
the time by the offset.

Closes mojombo#147
@stanhu
Copy link

stanhu commented Nov 23, 2019

I think #396 fixes this problem.

@aviisekh
Copy link

aviisekh commented Mar 20, 2020

Facing the same issue. Parsing is advance me to the next day. See the screenshot
Selection_009

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants