r85497 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r85496‎ | r85497 | r85498 >
Date:00:56, 6 April 2011
Author:mah
Status:deferred (Comments)
Tags:
Comment:
Patch from Bug #28406:
# the digitClass join versions were inverted: for a maxLength > 1 it should be
(|||), not []
# the regexp escape for digits in the ts_number_transform_table was a bit, er,
strange. Although it worked, I think my version is more common.
# most important: As recommended in http://en.wikipedia.org/wiki/Percent_sign,
there may be some whitespaces between the number and the
"%". See also Bug #15422 for that, I'm not sure wheter it's
included there.

I didn't include the “non-breaking space is part of \s” bit since I didn't want to mess up browser bug work-arounds
Modified paths:
  • /trunk/phase3/skins/common/wikibits.js (modified) (history)

Diff [purge]

Index: trunk/phase3/skins/common/wikibits.js
——@@ -740,17 +740,16 @@
741741 for ( var digit in ts_number_transform_table ) {
742742 // Escape regex metacharacters
743743 digits.push(
744 - digit.replace( /[\\\\$\*\+\?\.\(\)\|\{\}\[\]\-]/,
745 - function( s ) { return '\\' + s; } )
 744+ digit.replace( /([{}()|.?*+^$\[\]\\-])/g, "\\$1" )
746745 );
747746 if ( digit.length > maxDigitLength ) {
748747 maxDigitLength = digit.length;
749748 }
750749 }
751750 if ( maxDigitLength > 1 ) {
752 - var digitClass = '[' + digits.join( '', digits ) + ']';
 751+ var digitClass = '(' + digits.join( '|' ) + ')';
753752 } else {
754 - var digitClass = '(' + digits.join( '|', digits ) + ')';
 753+ var digitClass = '[' + digits.join( '' ) + ']';
755754 }
756755 }
757756
——@@ -760,7 +759,7 @@
761760 "^(" +
762761 "[-+\u2212]?[0-9][0-9,]*(\\.[0-9,]*)?(E[-+\u2212]?[0-9][0-9,]*)?" + // Fortran-style scientific
763762 "|" +
764 - "[-+\u2212]?" + digitClass + "+%?" + // Generic localised
 763+ "[-+\u2212]?" + digitClass + "+[\s\xa0]*%?" + // Generic localised
765764 ")$", "i"
766765 );
767766 };

Comments

#Comment by Brion VIBBER (talk | contribs)   23:27, 21 June 2011

Sortable table stuff, needs qunit tests.

#Comment by MarkAHershberger (talk | contribs)   14:55, 22 June 2011

Is the FIXME just to get the tests added? Could you remove it and leave just the "needs-js-test" tag to indicate that?

#Comment by Brion VIBBER (talk | contribs)   17:39, 22 June 2011

I could, but I'd rather actually require the tests to get done.

#Comment by MarkAHershberger (talk | contribs)   17:45, 22 June 2011

fair enough.

/me grumps about code he commits for others being marked FIXME

Guess that is reason enough for me to spread commit privs far and wide ;)

#Comment by DieBuche (talk | contribs)   18:31, 22 June 2011

This whole code was replaced by my rewrite

Status & tagging log