r73026 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r73025‎ | r73026 | r73027 >
Date:22:28, 14 September 2010
Author:tparscal
Status:resolved (Comments)
Tags:
Comment:
* Refactored mediaWiki.msg to use the configuration prototype
* Made basic parser extendable without having to replace the msg object
* Cleaned up whitespace and comments
Modified paths:
  • /trunk/phase3/resources/mediawiki/mediawiki.js (modified) (history)

Diff [purge]

Index: trunk/phase3/resources/mediawiki/mediawiki.js
@@ -55,8 +55,11 @@
5656 * An object which allows single and multiple get/set/exists functionality on a list of key / value pairs
5757 *
5858 * @param {boolean} global whether to get/set/exists values on the window object or a private object
 59+ * @param {function} parser function to perform extra processing before while getting a value which accepts
 60+ * value and options parameters where value is a string to be parsed and options is an object of options for the
 61+ * parser
5962 */
60 - 'configuration': function( global ) {
 63+ 'configuration': function( global, parser ) {
6164
6265 /* Private Members */
6366
@@ -66,52 +69,78 @@
6770 /* Public Methods */
6871
6972 /**
70 - * Gets one or multiple configuration values using a key and an optional fallback or an array of keys
 73+ * Gets one or more values
 74+ *
 75+ * If called with no arguments, all values will be returned. If a parser is in use, no parsing will take
 76+ * place when calling with no arguments or calling with an array of names.
 77+ *
 78+ * @param {mixed} selection string name of value to get, array of string names of values to get, or object
 79+ * of name/option pairs
 80+ * @param {object} options optional set of options which are also passed to a parser if in use; only used
 81+ * when selection is a string
 82+ * @format options
 83+ * {
 84+ * // Value to use if key does not exist
 85+ * 'fallback': ''
 86+ * }
7187 */
72 - this.get = function( keys, fallback ) {
73 - if ( typeof keys === 'object' ) {
74 - var result = {};
75 - for ( var k = 0; k < keys.length; k++ ) {
76 - if ( typeof values[keys[k]] !== 'undefined' ) {
77 - result[keys[k]] = values[keys[k]];
 88+ this.get = function( selection, options ) {
 89+ if ( typeof selection === 'object' ) {
 90+ var results = {};
 91+ for ( s in selection ) {
 92+ if ( selection.hasOwnProperty( s ) ) {
 93+ if ( typeof s === 'string' ) {
 94+ return that.get( values[s], selection[s] );
 95+ } else {
 96+ return that.get( selection[s] );
 97+ }
7898 }
7999 }
80 - return result;
81 - } else if ( typeof keys === 'string' ) {
82 - if ( typeof values[keys] === 'undefined' ) {
83 - return typeof fallback !== 'undefined' ? fallback : null;
 100+ return results;
 101+ } else if ( typeof selection === 'string' ) {
 102+ if ( typeof values[selection] === 'undefined' ) {
 103+ return 'fallback' in options !== 'undefined' ? options.fallback : null;
84104 } else {
85 - return values[keys];
 105+ if ( typeof parser === 'function' ) {
 106+ return parser( values[selection], options );
 107+ } else {
 108+ return values[selection];
 109+ }
86110 }
87111 } else {
88112 return values;
89113 }
90114 };
 115+
91116 /**
92117 * Sets one or multiple configuration values using a key and a value or an object of keys and values
 118+ *
 119+ * @param {mixed} key string of name by which value will be made accessible, or object of name/value pairs
 120+ * @param {mixed} value optional value to set, only in use when key is a string
93121 */
94 - this.set = function( keys, value ) {
95 - if ( typeof keys === 'object' ) {
96 - for ( var k in keys ) {
97 - values[k] = keys[k];
 122+ this.set = function( selection, value ) {
 123+ if ( typeof selection === 'object' ) {
 124+ for ( var s in selection ) {
 125+ values[s] = selection[s];
98126 }
99 - } else if ( typeof keys === 'string' && typeof value !== 'undefined' ) {
100 - values[keys] = value;
 127+ } else if ( typeof selection === 'string' && typeof value !== 'undefined' ) {
 128+ values[selection] = value;
101129 }
102130 };
 131+
103132 /**
104133 * Checks if one or multiple configuration fields exist
105134 */
106 - this.exists = function( keys ) {
 135+ this.exists = function( selection ) {
107136 if ( typeof keys === 'object' ) {
108 - for ( var k = 0; k < keys.length; k++ ) {
109 - if ( !( keys[k] in values ) ) {
 137+ for ( var s = 0; s < selection.length; s++ ) {
 138+ if ( !( selection[s] in values ) ) {
110139 return false;
111140 }
112141 }
113142 return true;
114143 } else {
115 - return keys in values;
 144+ return selection in values;
116145 }
117146 };
118147 }
@@ -123,12 +152,14 @@
124153 * Dummy function which in debug mode can be replaced with a function that does something clever
125154 */
126155 this.log = function() { };
 156+
127157 /*
128158 * List of configuration values
129159 *
130160 * In legacy mode the values this object wraps will be in the global space
131161 */
132162 this.config = new this.prototypes.configuration( LEGACY_GLOBALS );
 163+
133164 /*
134165 * Information about the current user
135166 */
@@ -138,44 +169,31 @@
139170
140171 this.options = new that.prototypes.configuration();
141172 } )();
 173+
142174 /*
143 - * Localization system
 175+ * Basic parser, can be replaced with something more robust
144176 */
145 - this.msg = new ( function() {
146 -
147 - /* Private Members */
148 -
149 - var that = this;
150 - // List of localized messages
151 - var messages = {};
152 -
153 - /* Public Methods */
154 -
155 - this.set = function( keys, value ) {
156 - if ( typeof keys === 'object' ) {
157 - for ( var k in keys ) {
158 - messages[k] = keys[k];
159 - }
160 - } else if ( typeof keys === 'string' && typeof value !== 'undefined' ) {
161 - messages[keys] = value;
 177+ this.parser = function( key, args ) {
 178+ if ( !( key in messages ) ) {
 179+ return '<' + key + '>';
 180+ }
 181+ var msg = messages[key];
 182+ if ( typeof args == 'object' || typeof args == 'array' ) {
 183+ for ( var a = 0; a < args.length; a++ ) {
 184+ msg = msg.replace( '\$' + ( parseInt( a ) + 1 ), args[a] );
162185 }
163 - };
164 - this.get = function( key, args ) {
165 - if ( !( key in messages ) ) {
166 - return '<' + key + '>';
167 - }
168 - var msg = messages[key];
169 - if ( typeof args == 'object' || typeof args == 'array' ) {
170 - for ( var a = 0; a < args.length; a++ ) {
171 - msg = msg.replace( '\$' + ( parseInt( a ) + 1 ), args[a] );
172 - }
173 - } else if ( typeof args == 'string' || typeof args == 'number' ) {
174 - msg = msg.replace( '$1', args );
175 - }
176 - return msg;
177 - };
178 - } )();
 186+ } else if ( typeof args == 'string' || typeof args == 'number' ) {
 187+ msg = msg.replace( '$1', args );
 188+ }
 189+ return msg;
 190+ };
 191+
179192 /*
 193+ * Localization system
 194+ */
 195+ this.msg = new that.prototypes.configuration( false, this.parser );
 196+
 197+ /*
180198 * Client-side module loader which integrates with the MediaWiki ResourceLoader
181199 */
182200 this.loader = new ( function() {
@@ -236,6 +254,7 @@
237255 pad1( date.getUTCSeconds() ) +
238256 'Z';
239257 }
 258+
240259 /**
241260 * Recursively resolves dependencies and detects circular references
242261 */
@@ -263,6 +282,7 @@
264283 resolved[resolved.length] = module;
265284 unresolved.splice( unresolved.indexOf( module ), 1 );
266285 }
 286+
267287 /**
268288 * Gets a list of modules names that a module dependencies in their proper dependency order
269289 *
@@ -292,6 +312,7 @@
293313 }
294314 throw new Error( 'Invalid module argument: ' + module );
295315 };
 316+
296317 /**
297318 * Narrows a list of module names down to those matching a specific state. Possible states are 'undefined',
298319 * 'registered', 'loading', 'loaded', or 'ready'
@@ -326,6 +347,7 @@
327348 }
328349 return list;
329350 }
 351+
330352 /**
331353 * Executes a loaded module, making it ready to use
332354 *
@@ -393,6 +415,7 @@
394416 }
395417 }
396418 }
 419+
397420 /**
398421 * Adds a dependencies to the queue with optional callbacks to be run when the dependencies are ready or fail
399422 *
@@ -518,6 +541,7 @@
519542 }
520543 }
521544 };
 545+
522546 /**
523547 * Registers a module, letting the system know about it and it's dependencies. loader.js files contain calls
524548 * to this function.
@@ -555,6 +579,7 @@
556580 registry[module].dependencies = dependencies;
557581 }
558582 };
 583+
559584 /**
560585 * Implements a module, giving the system a course of action to take upon loading. Results of a request for
561586 * one or more modules contain calls to this function.
@@ -594,6 +619,7 @@
595620 request( module );
596621 }
597622 };
 623+
598624 /**
599625 * Executes a function as soon as one or more required modules are ready
600626 *
@@ -630,6 +656,7 @@
631657 request( dependencies, ready, error );
632658 }
633659 };
 660+
634661 /**
635662 * Loads one or more modules for future use
636663 */
@@ -658,6 +685,7 @@
659686 return true;
660687 }
661688 };
 689+
662690 /**
663691 * Flushes the request queue and begin executing load requests on demand
664692 */
@@ -665,6 +693,7 @@
666694 suspended = false;
667695 that.work();
668696 };
 697+
669698 /**
670699 * Changes the state of a module
671700 *

Follow-up revisions

RevisionCommit summaryAuthorDate
r74080Resolved issues in r73026, namely a confusing comment and a missing var keyword.tparscal17:39, 1 October 2010

Comments

#Comment by Catrope (talk | contribs)   14:08, 1 October 2010
+		 * @param {function} parser function to perform extra processing before while getting a value which accepts
+		 * value and options parameters where value is a string to be parsed and options is an object of options for the
+		 * parser

This sentence is so weird ("before while"?) and unpunctuated that I needed to read it three times before understanding what it means. Could also use rephrasing to explicitly mention the signature à la function( value, options ). The latter might be a nice feature to build into your documentation system thingy.

+			 * @format options
+			 * 	{
+			 * 		// Value to use if key does not exist
+			 * 		'fallback': ''
+			 * 	}

I suggest you explicitly mention (again, it's also in $param, I know) that options also holds any additional options the caller may want to pass to the parser, perhaps in the form of a comment like the "Value to use" one.

+					for ( s in selection ) {

Use var s.

+						return 'fallback' in options !== 'undefined' ? options.fallback : null;

This seems to be the result of confusing 'fallback' in options with typeof options.fallback !== 'undefined'.

#Comment by Trevor Parscal (WMF) (talk | contribs)   17:43, 1 October 2010

r74080 resolves the first two issues, and the third has already been resolved (r73151)

Status & tagging log