mojombo / chronic

Chronic is a pure Ruby natural language date parser.
http://injekt.github.com/chronic
MIT License
3.24k stars 465 forks source link

:context => :past mostly ignored when parsing time (6am, 2:00) #60

Open troy opened 13 years ago

troy commented 13 years ago

Parsing 6pm finds a future time regardless of context, and so do many or most times without meridiem (6:00).

>> Time.now
=> Mon Aug 08 09:13:01 -0700 2011

Past parses to 9 hours ahead:

?> Chronic.parse('6pm',:context => :past)
=> Mon Aug 08 18:00:00 -0700 2011

that is, the same as future:

>> Chronic.parse('6pm', :context => :future)
=> Mon Aug 08 18:00:00 -0700 2011

Providing a time with no meridiem parses correctly in the past:

>> Chronic.parse('6:00', :context => :past)
=> Mon Aug 08 06:00:00 -0700 2011

.. but I believe it may be because 6 AM in the past happens to be closer to current time. When I try a different time without a meridiem, it chooses a future one and ignores context:

>> Chronic.parse('2:00', :context => :past)
=> Mon Aug 08 14:00:00 -0700 2011

All tests with 0.6.2. Thanks.

leejarvis commented 13 years ago

I've managed to get 10 minutes to look at this. This problem is actually a little more widespread than first though. First of all, Chronic ignores context in day portions. Now you can probably see the problem with a patch like this. Parsing, for example, 24th of January at 7pm with a past context will return the 23rd of January. Chronic isn't (yet) clever enough to know when it's parsing a time/day portion and nothing else. Chronic loops through Repeaters and re-uses code where it can, meaning it's clever enough to know that a month in the past means 1 month ago, or a year in the past means 1 year ago, but not that a 6pm in the past means yesterday at 6pm (unless it's later than 6pm today).

I'll mark this as a bug and try to find some time in the coming days to take a closer look at it. Thanks for reporting

troy commented 13 years ago

Thanks @injekt, I appreciate it. That all makes sense.

For any other readers, I think that for user-submitted timestamps, the viable solutions today are supporting a few specific formats, or presenting the parsed version back to the user so they can see the parsed time (ideally in realtime). "Enter what you want" is not feasible, nor is presenting errors on submission (some of the times do parse, they just aren't what the user expected).

leejarvis commented 13 years ago

OK so I think I need more user input into this issue. I mean, ignoring context for day portions kinda makes sense, but not in all situations. The trouble is, Chronic defaults to a future context when none is supplied, rather than :none, for example. But ignores this for day portions without more textual context. Meaning parsing 2pm at 3pm will return one hour previous, returning tomorrows 2pm would simply make no sense. But I also understand that at 11pm, 7am is more likely to mean tomorrow. There's a couple things we can do about this, either default to a non-specific context, and have the user supply a context when dealing with day portions, or leave it how it is, and assume the user will provide more information (for example 7am tomorrow, 10pm yesterday etc). Or alternatively have a cut off time, but I'm against that because it'll get extremely confusing for users.

troy commented 13 years ago

When context is not provided, I could see the default going either way. For the default, I think it's more important to document how it behaves than to pick the exact right case - as you said, there isn't one right default for everyone.

However, it should never ignore context when explicitly provided. That was the origin of this issue, and I think a lot more important than getting the default right. If the app developer is willing to spend the effort to decide authoritatively which context is correct for them, it should be honored.

I'd argue that it should raise an exception when it can't honor the context (rather than, say, returning a future time with context of past). Obviously that's not going to happen right now given that it ignores context for certain inputs, but I think that is the correct behavior when one is given. As a library consumer, I don't ever want a future time returned :)

dlupu commented 12 years ago

Hello

I have a similar issue (or ar least it seems).

Chronic.parse('27/7/2011 00:43:57', :context => :future) returns 2011-07-27 00:43:57 +0200 Chronic.parse('27/7/2011 00:43:57', :context => :past) returns 2011-07-26 00:43:57 +0200

I need context => :past in my application. In your opinion, the result of the second example is a bug or it's an expected behavior?

leejarvis commented 12 years ago

@dlupu Unless I'm missing something, I can't make much sense of your issue.

Chronic.parse('27/7/2011 00:43:57', :context => :past) returns 2011-07-26 00:43:57 +0200

Surely, with a past content, this is exactly what you're expecting?

dlupu commented 12 years ago

Sorry about not being clear @injekt. In fact I'm surprised to see that :context => :past changes the result in this particular case.

The parsed string is not ambiguous at all, so one could expect that both versions return the same result. I don't understand why context information changes the result in this case.

since the string to be parsed is pretty clear

leejarvis commented 12 years ago

Sorry I'm confused, looking at your examples you say that the first one returns 2011-07-27 00:43:57 +0200 and the last one returns 2011-07-26 00:43:57 +0200, these are both different. Am I missing something?

leejarvis commented 12 years ago

Oh wait, sorry I read your message wrong. You expect them to be the same? You didn't tell me what Time.now was to go by :+1:

dlupu commented 12 years ago

I did not specify any reference time so I guess Chronic uses the current time in that case.

leejarvis commented 12 years ago

Right, which was what? :P

dlupu commented 12 years ago

About 23 hours ago => Mon, 28 Nov 2011 21:07:15 CET +01:00

wulftone commented 12 years ago

I think he means this:

Time.now #=> 2011-12-29 12:03:14 -0800
Chronic.parse('1-12-1950 00:40', :context => :future) #=> 1950-01-12 00:40:00 -0800
Chronic.parse('1-12-1950 00:40', :context => :past) #=> 1950-01-11 00:40:00 -0800

The context is clearly irrelevant here, since the date is explicit. Context should not affect the date, right?

Or else, context should always affect the date, in which case the :future example should return Jan 13th... but that assumes that :future or :past always implies "a day ahead or a day behind" as opposed to a year or a century, or whatever.

leejarvis commented 12 years ago

No, you're correct. Context should not affect the date. It doesn't matter if we're in 1959 or 2059, 2011 is still the same year, that with any other attributes you give to Chronic, the date returned is supposed to be contextual, context is useless with an absolute date; or should be, at least.

Trying to find some time over the holidays to hack on Chronic but I'm finding it hard. I will get around to fixing this!

cpmurphy commented 11 years ago

@dlupu , I think my patch for issue #176 addresses your problem. With current master, I see

Time.now
=> 2013-04-15 11:56:35 -0500
Chronic.parse('27/7/2011 00:43:57', :context => :past)
=> 2011-07-27 00:43:57 -0500

@wulftone your clarifying examples no longer seem to work -- I just get nil, but

Chronic.parse('1/12/1950 00:40', :context => :past)

now returns 1950-01-12 00:40:00 -0600 as I'd expect (for my timezone).

BTW, @injekt thank you for merging my pull request!

Unfortunately, I think the original bug described by this issue is still present:

Time.now
=> 2013-04-15 12:05:21 -0500
Chronic.parse('6pm', :context => :past)
=> 2013-04-15 18:00:00 -0500