r97962 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r97961‎ | r97962 | r97963 >
Date:22:17, 23 September 2011
Author:catrope
Status:resolved (Comments)
Tags:
Comment:
Make Language::formatTimePeriod() more flexible so it can produce stuff like '3 hours ago'.

* Add a $noAbbrevs parameter that causes the 'seconds', 'minutes', etc. messages to be used instead of the 'seconds-abbrev', 'minutes-abbrev', etc. messages
* Add the 'seconds', 'minutes', 'hours' and 'days' messages
* Change the -abbrev messages to take a parameter rather than having the number prepended to them. This is for compatibility with 'seconds' et al, which need the parameter for {{PLURAL:}}. It also generally makes more sense. This does BREAK the messages in non-English languages that override them; Niklas told me to leave this alone and ping the TranslateWiki folks
* Introduce an 'ago' message for '$1 ago'. Not currently used in core, but I want to use it in an extension and it seemed stupid not to have such a thing in core.
* Refactor the function to use message objects and pass the number as a parameter
* Add tests! They exposed a subtle bug in my first iteration; all hail tests!
Modified paths:
  • /trunk/phase3/languages/Language.php (modified) (history)
  • /trunk/phase3/languages/messages/MessagesEn.php (modified) (history)
  • /trunk/phase3/tests/phpunit/languages/LanguageTest.php (modified) (history)

Diff [purge]

Index: trunk/phase3/tests/phpunit/languages/LanguageTest.php
——@@ -21,27 +21,42 @@
2222 }
2323
2424 /** @dataProvider provideFormattableTimes */
25 - function testFormatTimePeriod( $seconds, $avoid, $expected, $desc ) {
26 - $this->assertEquals( $expected, $this->lang->formatTimePeriod( $seconds, $avoid ), $desc );
 25+ function testFormatTimePeriod( $seconds, $avoid, $noAbbrevs, $expected, $desc ) {
 26+ $this->assertEquals( $expected, $this->lang->formatTimePeriod( $seconds, $avoid, $noAbbrevs ), $desc );
2727 }
2828
2929 function provideFormattableTimes() {
3030 return array(
31 - array( 9.45, false, '9.5s', 'formatTimePeriod() rounding (<10s)' ),
32 - array( 9.95, false, '10s', 'formatTimePeriod() rounding (<10s)' ),
33 - array( 59.55, false, '1m 0s', 'formatTimePeriod() rounding (<60s)' ),
34 - array( 119.55, false, '2m 0s', 'formatTimePeriod() rounding (<1h)' ),
35 - array( 3599.55, false, '1h 0m 0s', 'formatTimePeriod() rounding (<1h)' ),
36 - array( 7199.55, false, '2h 0m 0s', 'formatTimePeriod() rounding (>=1h)' ),
37 - array( 7199.55, 'avoidseconds', '2h 0m', 'formatTimePeriod() rounding (>=1h), avoidseconds' ),
38 - array( 7199.55, 'avoidminutes', '2h 0m', 'formatTimePeriod() rounding (>=1h), avoidminutes' ),
39 - array( 172799.55, 'avoidseconds', '48h 0m', 'formatTimePeriod() rounding (=48h), avoidseconds' ),
40 - array( 259199.55, 'avoidminutes', '3d 0h', 'formatTimePeriod() rounding (>48h), avoidminutes' ),
41 - array( 176399.55, 'avoidseconds', '2d 1h 0m', 'formatTimePeriod() rounding (>48h), avoidseconds' ),
42 - array( 176399.55, 'avoidminutes', '2d 1h', 'formatTimePeriod() rounding (>48h), avoidminutes' ),
43 - array( 259199.55, 'avoidseconds', '3d 0h 0m', 'formatTimePeriod() rounding (>48h), avoidseconds' ),
44 - array( 172801.55, 'avoidseconds', '2d 0h 0m', 'formatTimePeriod() rounding, (>48h), avoidseconds' ),
45 - array( 176460.55, false, '2d 1h 1m 1s', 'formatTimePeriod() rounding, recursion, (>48h)' ),
 31+ array( 9.45, false, false, '9.5s', 'formatTimePeriod() rounding (<10s)' ),
 32+ array( 9.45, false, 'noabbrevs', '9.5 seconds', 'formatTimePeriod() rounding (<10s)' ),
 33+ array( 9.95, false, false, '10s', 'formatTimePeriod() rounding (<10s)' ),
 34+ array( 9.95, false, 'noabbrevs', '10 seconds', 'formatTimePeriod() rounding (<10s)' ),
 35+ array( 59.55, false, false, '1m 0s', 'formatTimePeriod() rounding (<60s)' ),
 36+ array( 59.55, false, 'noabbrevs', '1 minute 0 seconds', 'formatTimePeriod() rounding (<60s)' ),
 37+ array( 119.55, false, false, '2m 0s', 'formatTimePeriod() rounding (<1h)' ),
 38+ array( 119.55, false, 'noabbrevs', '2 minutes 0 seconds', 'formatTimePeriod() rounding (<1h)' ),
 39+ array( 3599.55, false, false, '1h 0m 0s', 'formatTimePeriod() rounding (<1h)' ),
 40+ array( 3599.55, false, 'noabbrevs', '1 hour 0 minutes 0 seconds', 'formatTimePeriod() rounding (<1h)' ),
 41+ array( 7199.55, false, false, '2h 0m 0s', 'formatTimePeriod() rounding (>=1h)' ),
 42+ array( 7199.55, false, 'noabbrevs', '2 hours 0 minutes 0 seconds', 'formatTimePeriod() rounding (>=1h)' ),
 43+ array( 7199.55, 'avoidseconds', false, '2h 0m', 'formatTimePeriod() rounding (>=1h), avoidseconds' ),
 44+ array( 7199.55, 'avoidseconds', 'noabbrevs', '2 hours 0 minutes', 'formatTimePeriod() rounding (>=1h), avoidseconds' ),
 45+ array( 7199.55, 'avoidminutes', false, '2h 0m', 'formatTimePeriod() rounding (>=1h), avoidminutes' ),
 46+ array( 7199.55, 'avoidminutes', 'noabbrevs', '2 hours 0 minutes', 'formatTimePeriod() rounding (>=1h), avoidminutes' ),
 47+ array( 172799.55, 'avoidseconds', false, '48h 0m', 'formatTimePeriod() rounding (=48h), avoidseconds' ),
 48+ array( 172799.55, 'avoidseconds', 'noabbrevs', '48 hours 0 minutes', 'formatTimePeriod() rounding (=48h), avoidseconds' ),
 49+ array( 259199.55, 'avoidminutes', false, '3d 0h', 'formatTimePeriod() rounding (>48h), avoidminutes' ),
 50+ array( 259199.55, 'avoidminutes', 'noabbrevs', '3 days 0 hours', 'formatTimePeriod() rounding (>48h), avoidminutes' ),
 51+ array( 176399.55, 'avoidseconds', false, '2d 1h 0m', 'formatTimePeriod() rounding (>48h), avoidseconds' ),
 52+ array( 176399.55, 'avoidseconds', 'noabbrevs', '2 days 1 hour 0 minutes', 'formatTimePeriod() rounding (>48h), avoidseconds' ),
 53+ array( 176399.55, 'avoidminutes', false, '2d 1h', 'formatTimePeriod() rounding (>48h), avoidminutes' ),
 54+ array( 176399.55, 'avoidminutes', 'noabbrevs', '2 days 1 hour', 'formatTimePeriod() rounding (>48h), avoidminutes' ),
 55+ array( 259199.55, 'avoidseconds', false, '3d 0h 0m', 'formatTimePeriod() rounding (>48h), avoidseconds' ),
 56+ array( 259199.55, 'avoidseconds', 'noabbrevs', '3 days 0 hours 0 minutes', 'formatTimePeriod() rounding (>48h), avoidseconds' ),
 57+ array( 172801.55, 'avoidseconds', false, '2d 0h 0m', 'formatTimePeriod() rounding, (>48h), avoidseconds' ),
 58+ array( 172801.55, 'avoidseconds', 'noabbrevs', '2 days 0 hours 0 minutes', 'formatTimePeriod() rounding, (>48h), avoidseconds' ),
 59+ array( 176460.55, false, false, '2d 1h 1m 1s', 'formatTimePeriod() rounding, recursion, (>48h)' ),
 60+ array( 176460.55, false, 'noabbrevs', '2 days 1 hour 1 minute 1 second', 'formatTimePeriod() rounding, recursion, (>48h)' ),
4661 );
4762
4863 }
Index: trunk/phase3/languages/messages/MessagesEn.php
——@@ -3656,10 +3656,15 @@
36573657
36583658 # Video information, used by Language::formatTimePeriod() to format lengths in the above messages
36593659 'video-dims' => '$1, $2×$3', # only translate this message to other languages if you have to change it
3660 -'seconds-abbrev' => 's', # only translate this message to other languages if you have to change it
3661 -'minutes-abbrev' => 'm', # only translate this message to other languages if you have to change it
3662 -'hours-abbrev' => 'h', # only translate this message to other languages if you have to change it
3663 -'days-abbrev' => 'd', # only translate this message to other languages if you have to change it
 3660+'seconds-abbrev' => '$1s', # only translate this message to other languages if you have to change it
 3661+'minutes-abbrev' => '$1m', # only translate this message to other languages if you have to change it
 3662+'hours-abbrev' => '$1h', # only translate this message to other languages if you have to change it
 3663+'days-abbrev' => '$1d', # only translate this message to other languages if you have to change it
 3664+'seconds' => '{{PLURAL:$1|$1 second|$1 seconds}}',
 3665+'minutes' => '{{PLURAL:$1|$1 minute|$1 minutes}}',
 3666+'hours' => '{{PLURAL:$1|$1 hour|$1 hours}}',
 3667+'days' => '{{PLURAL:$1|$1 day|$1 days}}',
 3668+'ago' => '$1 ago',
36643669
36653670 # Bad image list
36663671 'bad_image_list' => 'The format is as follows:
Index: trunk/phase3/languages/Language.php
——@@ -3552,15 +3552,20 @@
35533553 * @param $format String Optional, one of ("avoidseconds","avoidminutes"):
35543554 * "avoidseconds" - don't mention seconds if $seconds >= 1 hour
35553555 * "avoidminutes" - don't mention seconds/minutes if $seconds > 48 hours
 3556+ * @param $noAbbrevs Bool If true (or true-ish, recommend using 'noabbrevs' for clarity), use 'seconds' and friends instead of 'seconds-abbrev' and friends
35563557 * @return string
35573558 */
3558 - function formatTimePeriod( $seconds, $format = false ) {
 3559+ function formatTimePeriod( $seconds, $format = false, $noAbbrevs = false ) {
 3560+ $secondsMsg = wfMessage( $noAbbrevs ? 'seconds' : 'seconds-abbrev' )->inLanguage( $this );
 3561+ $minutesMsg = wfMessage( $noAbbrevs ? 'minutes' : 'minutes-abbrev' )->inLanguage( $this );
 3562+ $hoursMsg = wfMessage( $noAbbrevs ? 'hours' : 'hours-abbrev' )->inLanguage( $this );
 3563+ $daysMsg = wfMessage( $noAbbrevs ? 'days' : 'days-abbrev' )->inLanguage( $this );
35593564 if ( round( $seconds * 10 ) < 100 ) {
35603565 $s = $this->formatNum( sprintf( "%.1f", round( $seconds * 10 ) / 10 ) );
3561 - $s .= $this->getMessageFromDB( 'seconds-abbrev' );
 3566+ $s = $secondsMsg->params( $s )->text();
35623567 } elseif ( round( $seconds ) < 60 ) {
35633568 $s = $this->formatNum( round( $seconds ) );
3564 - $s .= $this->getMessageFromDB( 'seconds-abbrev' );
 3569+ $s = $secondsMsg->params( $s )->text();
35653570 } elseif ( round( $seconds ) < 3600 ) {
35663571 $minutes = floor( $seconds / 60 );
35673572 $secondsPart = round( fmod( $seconds, 60 ) );
——@@ -3568,9 +3573,9 @@
35693574 $secondsPart = 0;
35703575 $minutes++;
35713576 }
3572 - $s = $this->formatNum( $minutes ) . $this->getMessageFromDB( 'minutes-abbrev' );
 3577+ $s = $minutesMsg->params( $this->formatNum( $minutes ) )->text();
35733578 $s .= ' ';
3574 - $s .= $this->formatNum( $secondsPart ) . $this->getMessageFromDB( 'seconds-abbrev' );
 3579+ $s .= $secondsMsg->params( $this->formatNum( $secondsPart ) )->text();
35753580 } elseif ( round( $seconds ) <= 2 * 86400 ) {
35763581 $hours = floor( $seconds / 3600 );
35773582 $minutes = floor( ( $seconds - $hours * 3600 ) / 60 );
——@@ -3583,12 +3588,11 @@
35843589 $minutes = 0;
35853590 $hours++;
35863591 }
3587 - $s = $this->formatNum( $hours ) . $this->getMessageFromDB( 'hours-abbrev' );
 3592+ $s = $hoursMsg->params( $this->formatNum( $hours ) )->text();
35883593 $s .= ' ';
3589 - $s .= $this->formatNum( $minutes ) . $this->getMessageFromDB( 'minutes-abbrev' );
 3594+ $s .= $minutesMsg->params( $this->formatNum( $minutes ) )->text();
35903595 if ( !in_array( $format, array( 'avoidseconds', 'avoidminutes' ) ) ) {
3591 - $s .= ' ' . $this->formatNum( $secondsPart ) .
3592 - $this->getMessageFromDB( 'seconds-abbrev' );
 3596+ $s .= ' ' . $secondsMsg->params( $this->formatNum( $secondsPart ) )->text();
35933597 }
35943598 } else {
35953599 $days = floor( $seconds / 86400 );
——@@ -3598,9 +3602,9 @@
35993603 $hours = 0;
36003604 $days++;
36013605 }
3602 - $s = $this->formatNum( $days ) . $this->getMessageFromDB( 'days-abbrev' );
 3606+ $s = $daysMsg->params( $this->formatNum( $days ) )->text();
36033607 $s .= ' ';
3604 - $s .= $this->formatNum( $hours ) . $this->getMessageFromDB( 'hours-abbrev' );
 3608+ $s .= $hoursMsg->params( $this->formatNum( $hours ) )->text();
36053609 } elseif ( $format === 'avoidseconds' ) {
36063610 $hours = floor( ( $seconds - $days * 86400 ) / 3600 );
36073611 $minutes = round( ( $seconds - $days * 86400 - $hours * 3600 ) / 60 );
——@@ -3612,15 +3616,15 @@
36133617 $hours = 0;
36143618 $days++;
36153619 }
3616 - $s = $this->formatNum( $days ) . $this->getMessageFromDB( 'days-abbrev' );
 3620+ $s = $daysMsg->params( $this->formatNum( $days ) )->text();
36173621 $s .= ' ';
3618 - $s .= $this->formatNum( $hours ) . $this->getMessageFromDB( 'hours-abbrev' );
 3622+ $s .= $hoursMsg->params( $this->formatNum( $hours ) )->text();
36193623 $s .= ' ';
3620 - $s .= $this->formatNum( $minutes ) . $this->getMessageFromDB( 'minutes-abbrev' );
 3624+ $s .= $minutesMsg->params( $this->formatNum( $minutes ) )->text();
36213625 } else {
3622 - $s = $this->formatNum( $days ) . $this->getMessageFromDB( 'days-abbrev' );
 3626+ $s = $daysMsg->params( $this->formatNum( $days ) )->text();
36233627 $s .= ' ';
3624 - $s .= $this->formatTimePeriod( $seconds - $days * 86400, $format );
 3628+ $s .= $this->formatTimePeriod( $seconds - $days * 86400, $format, $noAbbrevs );
36253629 }
36263630 }
36273631 return $s;

Sign-offs

UserFlagDate
Aaron Schulzinspected17:55, 24 September 2011
Aaron Schulztested17:55, 24 September 2011

Follow-up revisions

RevisionCommit summaryAuthorDate
r97965First stab at Special:MoodBarFeedback. Lacks functional paging, filtering, pe...catrope22:20, 23 September 2011
r97983Register new message keys from r97962raymond06:13, 24 September 2011
r98005Followup r97962: add qqq for new messages, and expand it for the -abbrev mess...catrope15:24, 24 September 2011
r98006Per CR on r97962, introduce an array parameter for formatTimePeriod() rather ...catrope15:44, 24 September 2011
r981381.17wmf1: MFT r97962, r98006, basically copies formatTimePeriod() from trunkcatrope15:22, 26 September 2011
r981441.17wmf1: MFT r97606, r97609, r97644, r97793, r97804, r97962, r98006, r98023catrope17:21, 26 September 2011

Comments

#Comment by Aaron Schulz (talk | contribs)   23:54, 23 September 2011

Maybe you should have just redid $format into $options as an associative array. If a string came in, for b/c it could set $options['avoid'].

Otherwise OK.

#Comment by Catrope (talk | contribs)   23:54, 23 September 2011

Makes sense. Will do that once I've had some sleep.

#Comment by Catrope (talk | contribs)   15:45, 24 September 2011

Done in r98006.

#Comment by Raymond (talk | contribs)   06:18, 24 September 2011

Could you please add some message documentation in the qqq section for translatewiki.net translators. Especially 'ago' is unclear outside the context of this commit.

#Comment by Catrope (talk | contribs)   15:45, 24 September 2011

Done in r98005.

#Comment by G.Hagedorn (talk | contribs)   20:18, 26 September 2011

since added to 1.18wmf, please consider tagging for 1.18 as well

#Comment by Mormegil (talk | contribs)   14:02, 27 September 2011

The ago message cannot work this way in flective languages, you need to have separate seconds, minutes, etc. messages for it. See translatewiki:Thread:Support/ago ("$1 ago") cannot work.

Status & tagging log