Re: [PEPr] +1 for Date and Time::Date_HumanDiff

On 2012-06-04 11:58, Christian Weiske wrote:

Hello Michael,

Michael Gauthier (http://pear.php.net/user/gauthierm) has voted +1 on the proposal for Date and Time::Date_HumanDiff. Vote information: http://pear.php.net/pepr/pepr-vote-show.php?id=678&handle=gauthierm

Thanks for the extensive review. You're welcome. :)

You've addressed all my concerns and my vote is no longer conditional. Cheers, Mike

1.) Method name setLocale() instead of setLanguage() makes more sense to me as you are passing in locales. I'd rename Lang classes as Locale at the same time.

Done.

2.) Does the sort method need to be static? I'd make it protected and dynamic so it can be overridden in subclasses.

Done.

3.) In makeTimestamp you use if ($foo) { return 'a'; } else { return 'b'; }. The else is redundant. Two separate if statements might be clearer.

Done.

4.) isIncludable() has error suppression operator which is not allowed in PEAR. Can you use file_exists + is_readable instead?

No, sorry. I need to see if the file exists in the include path, and using fopen + its third parameter is the pure php implementation of that functionality.

5.) Class docs for the Lang classes would be nice.

Done.

6.) What about languages with multiple plural forms? Will this work with Slocak or Arabic? Should this library even be responsible for translating strings? Another library like gettext or Zend_Translate seems like a better place to handle that.

Good catch. I'm passing the number now to the locale classes, which means that the implementation is completely free. Gettext and Zend_Translate backends are possible now.