r74503 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r74502‎ | r74503 | r74504 >
Date:10:49, 8 October 2010
Author:catrope
Status:ok (Comments)
Tags:
Comment:
Improve mediaWiki.parser() (which is a very strange name for a function that expands $1, $2, ... in i18n messages) to use .replace() with a callback, so it will handle edge cases like more than 9 parameters ($10 would be treated as $1 followed by a literal 0) or a parameter's value containing $n (which would then get substituted again). Code written by Neil Kandalgaonkar and trivially modified by me.
Modified paths:
  • /trunk/phase3/resources/mediawiki/mediawiki.js (modified) (history)

Diff [purge]

Index: trunk/phase3/resources/mediawiki/mediawiki.js
——@@ -175,9 +175,10 @@
176176 */
177177 this.parser = function( text, options ) {
178178 if ( typeof options === 'object' && typeof options.parameters === 'object' ) {
179 - for ( var p = 0; p < options.parameters.length; p++ ) {
180 - text = text.replace( '\$' + ( parseInt( p ) + 1 ), options.parameters[p] );
181 - }
 179+ text = text.replace( /\$(\d+)/g, function( str, match ) {
 180+ var index = parseInt( match, 10 ) - 1;
 181+ return index in options.parameters ? options.parameters[index] : '$' + match;
 182+ } );
182183 }
183184 return text;
184185 };

Comments

#Comment by Trevor Parscal (WMF) (talk | contribs)   18:51, 18 October 2010

I did some manual testing and this seems great, although we really need some unit tests for this sort of thing, does Neil have some?

#Comment by NeilK (talk | contribs)   19:09, 18 October 2010

no, but I think there will be soon (we'll have to add more functionality incl PLURAL, in order to add UploadWizard to the Resource Loader framework)

#Comment by Platonides (talk | contribs)   19:15, 18 October 2010

For what it does now, it should be called MsgReplaceArgs.

If it does more things, perhaps it could be called transform.

#Comment by Trevor Parscal (WMF) (talk | contribs)   19:25, 18 October 2010

The reason it's called parser is because is plugs into a hook which is expecting a parser of some sort. We are planning on adding a module that has more robust parsing capabilities, which would simply override this.

#Comment by Platonides (talk | contribs)   20:53, 18 October 2010

I don't disagree in that it is a "parser of some sort", but Parser in MediaWiki has a concrete meaning, and we shouldn't be overloading it (unless it replicated the whole parser, which is impractical). At least, it could be called jsparser or so.

Status & tagging log