matthewmueller / date

Date() for humans
http://matthewmueller.github.io/date/
1.48k stars 88 forks source link

"at X in the morning" defaults to 8am, rather than Xam #36

Closed catdad closed 11 years ago

catdad commented 11 years ago

Including morning after the time number overrides to 8am.

Examples: date("tomorrow morning") // tomorrow morning at 8am correct-ish date("tomorrow morning at 9") // tomorrow morning at 9am correct date("tomorrow at 9 in the morning") // tomorrow morning at 8am wrong

matthewmueller commented 11 years ago

oh interesting, yah this is definitely a bug. the morning default of 8am should have been overwritten in the last example

bulkan commented 11 years ago

I've been trying to look into fixing this bug, but getting stuck as the rAtHour wont run for date("tomorrow at 9 in the morning") wont find any matches.

@MatthewMueller any tips to where to look ?

matthewmueller commented 11 years ago

Does it execute: https://github.com/MatthewMueller/date/blob/master/lib/parser.js#L307 ?

bulkan commented 11 years ago

the function athour does execute but the rAtHour regex doesn't find any matches

the following prints null

var dateStr = "at 9 in morning"
var rAtHour = /^at\s?(\d{1,2})$/;
console.log(rAtHour.exec(dateStr));
matthewmueller commented 11 years ago

Ah okay, well that wouldn't execute because it has the $ on the end.

The athour function should receive the string at 9 though, which should match.

bulkan commented 11 years ago

Oh my example had the wrong dateStr

Now that you mention that, this.str progressively becomes the following;

test string is "tomorrow at 9 in the morning

at 9 in the morning
9 in the morning
in the morning
the morning

this explains why the rAtHour regex doesn't run. Looking at the execution of the functions in order

bulkan commented 11 years ago

@MatthewMueller i've fixed this issue by doing this https://github.com/bulkan/date/commit/749941dc0b0cf1434d9022c8d8f1b69f81122558.

Probably, a better way would be to peek back once from the number and see if the token is at

bulkan commented 11 years ago

Hey @MatthewMueller should I submit a pull request for the changes in https://github.com/bulkan/date/commit/749941dc0b0cf1434d9022c8d8f1b69f81122558 :question:

matthewmueller commented 11 years ago

@bulkan yep, then i can review