Feature Proposal: Replace Foswiki::Time with Time::ParseDate from CPAN

Motivation

After finding a bug in Foswiki::Time I started wondering why Foswiki needs its own time/date parser. It would seem more sensible to use a parser from CPAN, rather than waste our development efforts maintaining Foswiki::Time.

CPAN:Time::ParseDate does a very good job at parsing dates. Why don't we move to that?

Description and Documentation

I propose we replace our custom code in Foswiki::Time with Time::ParseDate. To maintain backwards compatibility, we can keep Foswiki::Time as a thin wrapper and continue to implement its functions in the same way.

Foswiki::Time does have some unit tests associated with it, so we can use them to ensure that nothing has changed in the way the module works.

Although Time::ParseDate is not a core Perl module, it does not seem to have any dependencies, so we can include it in the lib/CPAN directory.

Examples

Impact

WhatDoesItAffect:

%WHATDOESITAFFECT%

edit

Implementation

-- Contributors: AndrewJones - 12 Oct 2009

Discussion

This would be a good solution to move away from the broken parseTime (returns 0 when date is not valid), without breaking existing apps.

-- ArthurClemens - 12 Oct 2009

I do not support just adding dependency of Time::ParseDate

TablePlugin is part of standard Foswiki.

Time::ParseDate is not part of standard Perl which means noone can install Foswiki without knowing how to install yet-another-CPAN module. Especially Windows users have a hard time installing these modules. Even I have trouble at work because there is a nasty Firewall between my machine and the world that requires authentication etc. I have learned with time how to get around this but I can imagine how many that give up and never get Foswiki installed.

IF we are going to use this CPAN module we need to include it like we did with CGI::Session. If it is not a pure perl lib - forget it.

Besides - the problem we have should be a simple matter to solve.

There may also be other complications we have not considered. Last time someone fixed something in this area things broke in sorting of numbers. We can risk this module thinks an IP address and other patterns are dates.

And I bet the Time::ParseDate also has the problem of thinking 5/7 is 7th of May. And all us in the "rest-of-world" know this is 5th of July.

I have raised concern to make sure that if we proceeed with this

• Enough testing has been planned to make sure nothing else breaks.
• Time::ParseDate can be included in the local Foswiki CPAN lib
• Since already installed Time::ParseDate libs are found and used before our local Foswiki CPAN I also want some investigations in the bug history of Time::Parsedate to make sure we do not run into severe bugs we have to fight.
• Some confidence that this CPAN library has been created with an international mindset on date formats. I do not want x/y dates to suddenly be misinterpreted. Then it is better they are not seen as dates at all. There are too many serious confusions otherwise.

-- KennethLavrsen - 16 Oct 2009

IIRC Time::ParseDate requires a compiler to install, and I have had problems with it on several platforms (and in several versions). I'd rather a pure perl time parser, but can't find one. I beleive Time::ParseDate was written in Europe, so has a sensible view of 5/7.

-- CrawfordCurrie - 16 Oct 2009

I just checked Time::ParseDate as installed on my system. Seems to be all perl. Why does it need a compiler?

For the records: there has been a loooong history going back to the old project where people had strongly resisted in using too many cpan dependencies for the core. Strongest opponent those days: Pth ... a brick wall no head could ever pass. Fortunately we are Foswiki now!

Infact, not using arbitrary cpan modules is very reasonable as lots of them are crap. However, this only holds up to a certain degree. You better don't reinvent the wheel unless you are interested in wheels.

Me, I am strongly leaning towards reusing good code as much as possible even though it is provided by third parties. Of course there are problems in order to still provide a stable system given the added variables in the game. Other reasons besides better not to reinvent the wheel: (a) having more coders on board, even when it is indirect (b) free Foswiki resources for more important stuff (c) standardize (d) leaner Foswiki code base (might have missed some more importants).

From what I know as a full-time perl programmer Time::ParseDate is pretty respectable code that a lot of people use, more people than there are Foswiki users. So I would be very astonished if we'd run into quality problems with this module.

So this is a safe bet in away and a good candidate to be internalized if needed.

Btw. there are other areas we need to consider to fully cpanizing now that we got rid of dictators. What about Foswiki::Net?

-- MichaelDaum - 16 Oct 2009

I will be happy to extend the unit tests to ensure that it is well tested and it does the right thing with dates.

I mentioned above that I would like it shipped with Foswiki, rather than added the dependency. If it needs compiling then that is a problem, although Perl on Windows is getting a lot better with Strawberry Perl. Looking through the source and makefile of Time::ParseDate I can't see any C code, but I don't know how to be sure. Will see if I can find out.

The changelog can be found here. I don't know what serious bugs we have had before, so it will need someone who does to look through it.

Of course, the alternative is for someone to keep maintaining Foswiki::Time, but personally I see that as a waste of development effort. By using one from CPAN, the code is available to the entire Perl community and we can rely on them to help spot and fix bugs. In fact, I would rather add Time::ParseDate (or a similar CPAN module) as a dependency then fix Foswiki::Time.

-- AndrewJones - 16 Oct 2009

I'm all for cpanizing important basic modules, too, because this means that we can benefit from their stability that comes with larger user bases. And Time::ParseDate would make a very good first candidate.

-- MarkusUeberall - 16 Oct 2009

I've been using Time::ParseDate for years with the action tracker, without significant issues. I thought it was compiled, but if not, I have no objection to using it. I totally agree about Foswiki::Time, though note that some of the functionality (ISO8601 format dates, date ranges) does not seem to be supported by Time::ParseDate.

-- CrawfordCurrie - 16 Oct 2009

Hmm, if it doesn't support ISO dates then we will need to keep our implementation of it, and/or submit a patch upstream.

Obviously I will need to guarantee that Time::ParseDate supports all the date formats that Foswiki::Time supports, but im confident I will be able to (with unit tests to prove it).

-- AndrewJones - 16 Oct 2009

Foswiki must be possible to install without having to get professionel paid help installing it.

It is childtalk to say that because Pth had this view we should think the opposite.

• Kenneth, you know all the arguments. We went thru this a thousand times those days. -- MD

As long as we can include the lib in our local CPAN library there is no problem. It would be strange if Time::ParseDate would be binary. Its function is the exact kind of thing Perl is good at.

Andrew raised an even more important concern. Is the CPAN module capable of doing all the formats we have today?

The reason we have bug we have now in TablePlugin was because someone changed the code in the plugin from having its own time parser to using the one we have in Foswiki core (in itself a good idea). and did not check that all formats was supported. I am pretty sure the dd-mmm-yyyy format was supported in the old days. So IF we move to the CPAN library we must check ALL the supported formats. Our current unit tests do not cover all of them so they need to get extended also.

• These two "someones" were CDot and me. The final neck-breaker for sorting in TablePlugin was parseTime() accepting plain integers like 2009 as dates. Not sure why this is needed. Another problem is how TablePlugin decides a column is all integers or all dates. This smells as well. This was part of a big regex reorganization. This regex is pretty much unmaintainable today. At least I don't grok it. Best would be to dump this column sorting code and make it client-side javascript -- MD

I already fixed the problem with the dd-mm-yyyy in our Foswiki time parser. It is pretty simple code that is easy to overview.

If we end up having to pre-parse ourselves, and then call the CPAN module then I do not see the benefit.

Looking briefly at the code we have a problem in the current Foswiki time parsing that it is English focussed and we have an open todo to enable for example German month words to work. I cannot see the CPAN lib addressing this. And once we go with the CPAN we cannot fix this. We have to step carefully here.

-- KennethLavrsen - 17 Oct 2009

Added some comments inline .

-- MichaelDaum - 17 Oct 2009

The unit tests for Foswiki::Time are already quite complete. You can see the tests here. It covers all that Foswiki::Time says it supports. A quick Google did not find many more popular date/time formats, but if you can find any please add them here and I will extend the unit tests, even if we don't currently support them.

Also I would suggest that moving to a CPAN module does not prevent us from fixing it. We can still submit patches to it. I would also suggest that it is more likely that a CPAN module will add support for German month words than we will.

-- AndrewJones - 17 Oct 2009

A quick pointer regarding int'l date format support (haven't tried the modules in question myself, though): DateTime (Mailing list)

-- MarkusUeberall - 17 Oct 2009

I think we need to separate two issues here: (1) Foswiki possibly duplicating code that is available elsewhere (2) support for international date formats.

The Time unit tests were some of the first unit tests I wrote for TWiki. I can't be 100% certain, but I'm fairly confident that the parseable formats supported now are the same parseable formats that have always been supported. I have looked at using a CPAN module several times over the years, but never found anything that parsed all the formats that Foswiki::Time handled. There may be another module out there, and if there is I'd favour adopting it, but AFAIK Foswiki::Time is actually rather more functional than anything else. So, perhaps we should be thinking about making Foswiki::Time into a CPAN module, instead of the reverse.

It's easy to get carried away with international date formats, and try to be too clever. TWiki elected to standardise on ISO8601, and I think they made the right move there. While it might be nice to support free-format dates including international month names (and all the various character sets required to represent them) there are definite advantages to supporting only dates that use numbers. So, my inclination is to treat parsing for "other" date formats as a nice-to-have-for-compatibility, but to focus efforts on crisp and reliable support for ISO8601 - at least until a drop-in replacement for Foswiki::Time is available.

And focus our efforts on more pressing requirements elsewhere.

-- CrawfordCurrie - 18 Oct 2009

I would support making Foswiki::Time into a CPAN module. The main reason for making this feature request was not because Foswiki::Time is no good, but because it seemed strange to have our own implementation of something that is so generic.

-- AndrewJones - 21 Oct 2009

Still we have the issue that Foswiki::Time returns 0 for an invalid date (which is actually 1 Jan 1970), instead of an undefined. So something needs to get fixed before it is moved to CPAN. It seemed that using another, well tested CPAN module would solve the issue, but if that one does not support Foswiki's needs, we are back to square one.

-- ArthurClemens - 21 Oct 2009

Thats easy to fix, just replace return 0 with return undef. Presumably there is no argument against this fix? If not, I will do that tonight. (Tasks.Item2043)

-- AndrewJones - 21 Oct 2009

I cannot foresee what will fall over. Core code is (hopefully) unit tested, but some plugins use that function too.

-- ArthurClemens - 21 Oct 2009

The original proposal assumed the CPAN module have all the features of the Foswiki Time module or more.

During the discussion we got the information that our own module supports more formats than the CPAN module.

This in my view means that the proposal rejects itself as it is not possible.

So in an attempt to clean out the backlog of proposals I dare to declare this one rejected by concensus reached with the reason that it would not work technically without removing several time formats that we support today.

If anyone disagrees in this conclusion flip it back to UnderInvestigation

-- KennethLavrsen - 13 Nov 2009

There is an alternative interpretation of the discussion - that we need to provide a wrapper to the CPAN module to add our extra functionalities - which can either be brought into that CPAN module over time, or actually be another CPAN module.

but which interpretation depends mostly on Andrew and what he's willing to do.

-- SvenDowideit - 13 Nov 2009

It seems we are agreed that Foswiki::Time is more featureful than the alternative CPAN modules that were found.

It would be nice if we abstracted it out into a CPAN module for the benefit of the Perl community, which could also bring the other benefits that come with an increased user base (the reasons MichaelDaum mentioned earlier). For me, it does not make sense for Foswiki to have its own implementation for such a generic problem.

I would be willing to do that work. Perhaps the implementation would be:

• Extract Foswiki::Time into a new generic date/time module and upload to CPAN, under a new name
• That CPAN module would be shipped with Foswiki in the lib/CPAN directory (so we do not add another dependency)
• Keep Foswiki::Time as a thin wrapper (for backwards compatibility and plugins) but depreciate

Does this sound sensible and worthwhile to everyone else?

I switched it back to Under Investigation for discussion..

-- AndrewJones - 13 Nov 2009

Do Time::ParseDate and Foswiki::Time intersect in the number of formats or is one the true superset of the other? I guess the former is the case.

-- MichaelDaum - 13 Nov 2009

CPAN:Time::ParseDate is a time parsing module. Foswiki::Time is a time parsing and formatting module. The simple time parsing part of Foswiki::Time is about 100 lines of code, or about 15% of the code in the module, and handles a subset of ISO8601 and the TWiki "dd Mmm YYYY - hh:mm" format (since abandoned by TWiki in favour of ISO8601, but still used in Foswiki and in historical topics). As well as the parsing, Foswiki::Time handles formatting date/times in a range of formats, including delta times, and parsing of time ranges.

Time::ParseDate parses date formats including a subset of the "dd Mmm YYYY - hh:mm" format (it does the date and time separately, but not combined), but does not parse ISO8601. On the other hand it has rich support for relative date specifiers, such as "+100 minutes" and "midnight", which are not currently Foswiki requirements but could be seen as interesting features.

There are CPAN modules that handle all the individual requirements that Foswiki has, but there is no single module that handles the rather eclectic Foswiki mix. I suspect that to use CPAN modules to handle the individual requirements would require glue code that would probably be as big as the current Foswiki parsing solution.

-- CrawfordCurrie - 13 Nov 2009

Rejecting this proposal because of the concerns raised.

I still think extracting out generic code is a good idea, but am not going to commit to the idea, as I have other priorities.

-- AndrewJones - 17 Feb 2010

Commenting is disabled while not logged in