search

engaging-computing / phpSENSE

An educational data analytics platform.
http://isenseproject.org
14 stars 10 forks source link

Some issues with time parsing (AM/PM) #568

Closed mmcguinn closed 11 years ago

mmcguinn commented 11 years ago

It looks like sometimes 24hr time is parsed as AM/PM despite lacking either of those symbols. I don't have an example on hand, but I'm going to look into it.

mmcguinn commented 11 years ago

Here's an example of what failed previously:

"5 10 1990 12:45:30.1" => some time in october "5 10 1990 12:45:30.1 PM" => works

Now both should work.

mmcguinn commented 11 years ago

So this currently is not working for AM. Also I am going to attach a fix for the whole 'no negative numbers' problem.

mmcguinn commented 11 years ago

After consideration, the format "MM DD YYYY" is incompatible with the format "YYYY MM DD". Consider the date "5 12 10":

is it May 12 2010? is it May 12 1910? is it May 12 10? is it Dec 10 2005? is it Dec 10 1905? is it Dec 10 5?

@fgmart @ivanrudnicki @jdalphon We need to decided which of these two to use, since its impossible to tell them apart in some circumstances. Without thinking I'd jump to using "MM DD YYYY", but I think the uPPT currently uses "YYYY MM DD" (because of the similarity to certain ISO date formats).

fgmart-zz commented 11 years ago

Two things:

(1) I am totally OK with 2-digit years being not allowed.

(2) I don't understand why MM DD YYYY and YYYY MM DD can't both be supported? (other than annoyance factor for figuring out which is which)... in other words, isn't this a separate issue from trying to support a 2-digit year?

mmcguinn commented 11 years ago

The problem is I can't specify 'no two digit years' to either this parser, or any of the other ones we have previously used. It will match whatever fits the pattern, and 'YYYY' doesn't match a 4-digit year (despite its appearance) but matches ANY year.

On second thought though, I may be a bit too fixated on making the parser itself deal with this problem when it might be fairly easy to make an accurate guess as to which format the string is in beforehand. I can probably get something working on all non-ambiguous cases (and default to the most-sane looking one otherwise).

mmcguinn commented 11 years ago

So I've nearly got this working, but I've actually located a bug in moment.js that's preventing me from interpreting time as GMT (as opposed to whatever local time is). I've reported the bug here: https://github.com/timrwood/moment/issues/561

I will finish up without proper timezones for now, so all times will be off by the local timezone of the viewer, along with weirdness when daylight savings time kicks in/out. In the case of an ambiguous 1/2 digit year, it currently assumes a "MM DD YYYY" format, for logical convenience more than anything else.

mmcguinn commented 11 years ago

Note to testers: Keep in mind that everything will be off by your timezone. I've made a fairly comprehensive set of test data you can get here: https://www.wuala.com/Hunter0000/WualaSync/iSENSE/?key=vhbKDoEYDRcc

mmcguinn commented 11 years ago

The fix to moment introduced other problems that I was then unable to fix in turn.... I ended up deciding to make my own parser which in retrospect I should have known to begin with (should have listened to Derrell!). The above test data is still perfectly good, but testers please try various spacers (slashes, dashes, ect..) as well as invalid dates to help test the new parser.