r114067 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r114066‎ | r114067 | r114068 >
Date:20:58, 17 March 2012
Author:jeroendedauw
Status:reverted (Comments)
Tags:gerritmigration 
Comment:
adding a duration function to language that converts seconds to text
Modified paths:
  • /trunk/phase3/languages/Language.php (modified) (history)
  • /trunk/phase3/languages/messages/MessagesEn.php (modified) (history)

Diff [purge]

Index: trunk/phase3/languages/messages/MessagesEn.php
@@ -4820,4 +4820,13 @@
48214821 'api-error-uploaddisabled' => 'Uploading is disabled on this wiki.',
48224822 'api-error-verification-error' => 'This file might be corrupt, or have the wrong extension.',
48234823
 4824+# Durations
 4825+'duration-seconds' => '$1 {{PLURAL:$1|second|seconds}}',
 4826+'duration-minutes' => '$1 {{PLURAL:$1|minute|minutes}}',
 4827+'duration-hours' => '$1 {{PLURAL:$1|hour|hours}}',
 4828+'duration-days' => '$1 {{PLURAL:$1|day|days}}',
 4829+'duration-weeks' => '$1 {{PLURAL:$1|week|weeks}}',
 4830+'duration-years' => '$1 {{PLURAL:$1|year|years}}',
 4831+'duration-centuries' => '$1 {{PLURAL:$1|century|centuries}}',
 4832+
48244833 );
Index: trunk/phase3/languages/Language.php
@@ -1910,6 +1910,44 @@
19111911 }
19121912
19131913 /**
 1914+ * Takes a number of seconds and turns it into a text using values such as hours and minutes.
 1915+ *
 1916+ * @since 1.20
 1917+ *
 1918+ * @param integer $seconds The amount of seconds.
 1919+ * @param array $chosenIntervals The intervals to enable.
 1920+ *
 1921+ * @return string
 1922+ */
 1923+ public function duration( $seconds, array $chosenIntervals = array( 'years', 'days', 'hours', 'minutes', 'seconds' ) ) {
 1924+ $intervals = array(
 1925+ 'years' => 31557600, // 86400 * 365.25
 1926+ 'weeks' => 604800,
 1927+ 'days' => 86400,
 1928+ 'hours' => 3600,
 1929+ 'minutes' => 60,
 1930+ 'seconds' => 1,
 1931+ );
 1932+
 1933+ if ( !empty( $chosenIntervals ) ) {
 1934+ $intervals = array_intersect_key( $intervals, array_flip( $chosenIntervals ) );
 1935+ }
 1936+
 1937+ $segments = array();
 1938+
 1939+ foreach ( $intervals as $name => $length ) {
 1940+ $value = floor( $seconds / $length );
 1941+
 1942+ if ( $value > 0 || ( $name == 'seconds' && empty( $segments ) ) ) {
 1943+ $seconds -= $value * $length;
 1944+ $segments[] = wfMsgExt( 'duration-' . $name, 'parsemag', $value );
 1945+ }
 1946+ }
 1947+
 1948+ return $this->listToText( $segments );
 1949+ }
 1950+
 1951+ /**
19141952 * Internal helper function for userDate(), userTime() and userTimeAndDate()
19151953 *
19161954 * @param $type String: can be 'date', 'time' or 'both'

Follow-up revisions

RevisionCommit summaryAuthorDate
r114070follow up to r114067 and changed special:educationprogram to use new caching ...jeroendedauw21:07, 17 March 2012
r114071Follow up to r114067;jeroendedauw21:13, 17 March 2012
r114075follow up to r114067, use correct languagejeroendedauw21:47, 17 March 2012
r114079Followup r114067...reedy22:14, 17 March 2012
r114082Followup r114067 rename to formatDuration to match other methodsreedy22:26, 17 March 2012
r114084Unit tests for r114067reedy22:39, 17 March 2012
r114326Revert r114067, r114071, r114075, r114079, r114081, r114082, r114084, r114086......catrope23:03, 20 March 2012

Comments

#Comment by Bawolff (talk | contribs)   21:18, 17 March 2012
$segments[] = wfMsgExt( 'duration-' . $name, 'parsemag', $value );

So if someone does $wgContLang->duration( 10 ); it would return 10 seconds, with seconds in the user language instead of the content language? That doesn't seem right. The message should be in the same language as whatever language the language object being used represents.

#Comment by Jeroen De Dauw (talk | contribs)   21:48, 17 March 2012

Absolutely right - I forgot to finish that part up :) Now fixed by r114075

#Comment by Reedy (talk | contribs)   21:37, 17 March 2012

Also messages need adding to messages.inc

#Comment by Jeroen De Dauw (talk | contribs)   21:42, 17 March 2012

Was already fixed by r114071

#Comment by Reedy (talk | contribs)   21:38, 17 March 2012

Also, decade, millennia?

#Comment by Jeroen De Dauw (talk | contribs)   21:42, 17 March 2012

Feel free to add that.

#Comment by Reedy (talk | contribs)   22:07, 17 March 2012

You should probably add some unit tests to go with this too

#Comment by Jeroen De Dauw (talk | contribs)   22:33, 17 March 2012

Yeah, I thought about that. Is there a place with language unit tests yet?

#Comment by Bawolff (talk | contribs)   22:38, 17 March 2012

phase3/tests/phpunit/languages/LanguageTest.php

#Comment by Reedy (talk | contribs)   22:16, 17 March 2012

I know 365.25 is technically right, but should it really be 365?

Also, it really should be named formatDuration to match with formatTimePeriod, formatBitrate, formatSize...

#Comment by Jeroen De Dauw (talk | contribs)   22:32, 17 March 2012

Why would you not want to be technically right? 365.25 is not going to cause anyone getting confused... I doubt that most people will even notice :)

I'm fine w/ the name change. Language apparently is rather inconsistent :/

#Comment by Reedy (talk | contribs)   22:40, 17 March 2012

Yeah.. Bleugh.

I wonder if we should add some more methods for consistency...

#Comment by Jeroen De Dauw (talk | contribs)   22:56, 17 March 2012

Could do that. Although deprecating the already existing ones is going to cause compat hassle in extensions :/

#Comment by Reedy (talk | contribs)   23:00, 17 March 2012

Indeed. I suspect it's something that's not worth the hassle, there's no other gains really

#Comment by Jeroen De Dauw (talk | contribs)   23:05, 17 March 2012

Agreed.

So we need to properly fix these inconsistencies... what about time travel?

#Comment by Duplicatebug (talk | contribs)   09:37, 18 March 2012

Where is the difference between Language::formatDuration and Language::formatTimePeriod?

#Comment by Reedy (talk | contribs)   15:56, 18 March 2012

formatTimePeriod only does days... Whereas formatDuration now does upto millenia!

'seconds'        => '{{PLURAL:$1|$1 second|$1 seconds}}',
'minutes'        => '{{PLURAL:$1|$1 minute|$1 minutes}}',
'hours'          => '{{PLURAL:$1|$1 hour|$1 hours}}',
'days'           => '{{PLURAL:$1|$1 day|$1 days}}',
#Comment by Hashar (talk | contribs)   20:03, 22 March 2012

Jeroen, the migration of that subversion commit and all the subversion crippled Gerrit with a ton of changes. I am abandoning them, so you will have to reapply the various revisions. Contact me if needed.

#Comment by Catrope (talk | contribs)   21:42, 22 March 2012

I've resubmitted it as one huge commit: https://gerrit.wikimedia.org/r/#change,3497

#Comment by Hashar (talk | contribs)   22:24, 22 March 2012

Thanks Roan! I have tried squashing the commits twice this afternoon and failed .... a couple of time :-/

Now, we need to review all that patch.

#Comment by Catrope (talk | contribs)   22:25, 22 March 2012

The squashed patch corresponds to what I submitted to Gerrit from SVN, and doesn't contain any changes that may have been made in Gerrit later; those would have to be reapplied.

Status & tagging log