3/13/2016 12:35 AM returns nil

[jclusso]

This is a really odd issue. This time doesn't work but 3/13/2016 12:35 AM works fine. If you do 3/12/2017 12:35 AM it doesn't work either. I ran the code on every other day of the year and no issues so I'm mind boggled. The issue also doesn't exist if you change the time to 1 AM.

[davispuh]

I can't reproduce it and I suspect it's DST related, can you show example of how you parse it and which timezone is used?

[jclusso]

I made this script to show how I tested it. Also we parse like this. I'm not sure how it would be DST related and only affect 1 hour of 1 day per year. Also why would it return nil. DST shouldn't be causing it to return nil. The minute has no difference on it when I manually tested it.

Chronic.parse('3/13/2016 12:35 AM')

year = 2016
hours = []

12.times.each_with_index do |m|
  m = m + 1
  Time.days_in_month(m, year).times.each_with_index do |d|
    d = d + 1
    24.times.each_with_index do |h|
      h = h + 1
      am_or_pm = h > 12 ? 'PM' : 'AM'
      h = h - 12 if h > 12
      hours << Chronic.parse("#{m}/#{d}/#{year} #{h} #{am_or_pm}")
    end
  end
end

hours.select { |h| h.nil? }.count

[davispuh]

It's definitely DST related bug.

For me with EET timezone it's Chronic.parse('2016-03-27 12am') which gives nil and not your time.

Handler: handle_sy_sm_sd
Handler-class: Chronic::RepeaterDayPortion
--(2016-03-27 00:00:00 +0200..2016-03-27 11:59:59 +0300)

And it makes sense because in this day clock is turned +1h ahead so essentially there's 1 hour missing and thus nil because that hour can't exist, but there's bug that it's not 12am when it's +1h but from 3am becomes 4am and there isn't 3am.

Anyway it's known that Chronic have loads of DST issues and it's pretty much broken there, but I've a rewrite branch and there never returns nil, tested with your script and for DST change it returns same hour twice, so that 3am returns 4am for me because 3am doesn't exist.

[jclusso]

Just wondering, but how come it only affects 3/13/2016 and no other day?

[davispuh]

because DST change happens only in that day, in other day it moves -1h back thus same hour appears twice and it does exist unlike in this case.

[TimHolahan]

@davispuh Thanks for the responses, and for working on chronic.

Your rewrite branch looks like it solves the problem. I'd like to use it, but it seems to have a more basic issue with time-parsing. In the examples below, all created today (3/14/16), unless I explicitly indicate no minutes and no seconds, your gem adds 30 to the largest unspecified unit:

2.2.2 :002 > Chronic.parse('yesterday')
 => 2016-03-13 12:30:00 -0400
2.2.2 :003 > Chronic.parse('yesterday at 1 am')
 => 2016-03-13 01:30:00 -0500
2.2.2 :004 > Chronic.parse('yesterday at 3 am')
 => 2016-03-13 03:30:00 -0400
2.2.2 :005 > Chronic.parse('yesterday at 3:00 am')
 => 2016-03-13 03:00:30 -0400
2.2.2 :006 > Chronic.parse('yesterday at 3:00:00 am')
 => 2016-03-13 03:00:00 -0400
2.2.2 :007 > Chronic.parse('yesterday at 9 am')
 => 2016-03-13 09:30:00 -0400
2.2.2 :008 > Chronic.parse('today at 9 am')
 => 2016-03-14 09:30:00 -0400
2.2.2 :009 > Chronic.parse('today at 9:00 am')
 => 2016-03-14 09:00:30 -0400
2.2.2 :010 > Chronic.parse('today at 9:00:00 am')
 => 2016-03-14 09:00:00 -0400
2.2.2 :011 > Chronic.parse('tomorrow at 9 am')
 => 2016-03-15 09:30:00 -0400

Is this something you're aware of?

[davispuh]

you need add :guess => :begin option like Chronic.parse('today at 9:00 am', :guess => :begin)

it's because current Chronic version gets interval and takes middle of it but it did so very inconsistently and that was fixed there by always using correct interval range.

for example current Chronic

Chronic.parse('today', :guess => false)
=> 2016-03-15 02:00:00 +0200..2016-03-16 00:00:00 +0200
Chronic.parse('today')
=> 2016-03-15 13:00:00 +0200 # we're in middle of range

 Chronic.parse('today', :guess => :begin)
=> 2016-03-15 02:00:00 +0200

Chronic.parse('today at 7am', :guess => false)
=> 2016-03-15 07:00:00 +0200..2016-03-15 07:00:01 +0200 # it's second precision, but minutes weren't specified...
Chronic.parse('today at 7am')
=> 2016-03-15 07:00:00 +0200 # if "today" was middle, why not this?

with my rewrite branch it's always consistent

 Chronic.parse('today', :guess => false)
=> 2016-03-15 02:00:00 +0200...2016-03-16 00:00:00 +0200
 Chronic.parse('today')
=> 2016-03-15 13:00:0 +0200 # middle of today's range

Chronic.parse('today', :guess => :begin)
=> 2016-03-15 02:00:00 +0200

Chronic.parse('today at 7am', :guess => false)
=> 2016-03-15 07:00:00 +0200...2016-03-15 08:00:00 +0200 # now 1h precision
Chronic.parse('today at 7am')
=> 2016-03-15 07:30:00 +0200 # also middle of interval

Chronic.parse('today at 7:30am', :guess => false)
=> 2016-03-15 07:30:00 +0200...2016-03-15 07:31:00 +0200 # 1m precision because minutes specified

anyway that branch is still work in progress and while there's a lot of work done and basically almost all tests pass and there aren't expected to be big changes anymore there still might be some new bugs. But it does have fixed a lot of current Chronic's issues, especially regarding DST.
PRs always welcome and on that branch if there's no test for particular date/time format it's considered unsupported so need tests for all formats.

[TimHolahan]

Hi, @davispuh

I very much appreciate the detailed response. I had noticed some of the problems you mention about the inconsistent guessing, and it would be nice to have those fixed.

Still, though consistency is a good thing, it seems "off" from a UX point of view to have the default response to Chronic.parse("today at 9 am") return 9:30 am. Nevertheless, it's your project, so it's your call.

My guess is that it's probably a better idea to use your branch than the standard gem at this point, but with your cautionary words about the tests and date formats, and a little discomfort on my part about pointing a production build at a Github branch, I guess I'll stay with the standard gem for the moment.

Thanks for your work on this. I'm following your fork, and if you get your branch to a point at which you feel it's production-ready, I'll be eager to use it.

[davispuh]

It's that way only because it was that way before, I mean with current Chronic.parse('today'), if I change to :begin then it won't match previous behavior but yeah I agree that even before it was really stupid idea to and maybe it's worth changing to default which seems like good idea.

and yes, I can't really recommend using it yet, I myself wouldn't use it yet :D it needs much more testing and fixing some things. Only I don't know when I'll have time to work on it again, but all code is there up so everything's that there is there and any PRs would be great.

[davispuh]

btw about that work I did on rewrite branch see #278