r93702 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r93701‎ | r93702 | r93703 >
Date:03:11, 2 August 2011
Author:krinkle
Status:ok (Comments)
Tags:
Comment:
mediawiki.Title: Removing public setter functions. Title object should be immutable.
- To avoid repetition, instead of substituting, changed them to private helper functions
- Updated test suite to create new objects for different titles, rather than modifying the existing one.
- Adding missing unit tests, now 100% complete according to QUnit CompletnessTest
+ Test for Title.prototype.getNamespaceId
+ Test forTitle.prototype.getPrefixedText
+ Test forTitle.prototype.getExtension

Follows-up r90331 CR
Modified paths:
  • /trunk/phase3/resources/mediawiki/mediawiki.Title.js (modified) (history)
  • /trunk/phase3/tests/qunit/suites/resources/mediawiki/mediawiki.Title.js (modified) (history)

Diff [purge]

Index: trunk/phase3/tests/qunit/suites/resources/mediawiki/mediawiki.Title.js
——@@ -63,76 +63,56 @@
6464 ok( mw.Title, 'mw.Title defined' );
6565 });
6666
67 -test( 'Filename', function() {
68 - expect(4);
 67+test( 'Transform between Text and Db', function() {
 68+ expect(2);
6969 _titleConfig();
7070
71 - var title = new mw.Title( 'File:foo_bar.JPG' );
 71+ var title;
7272
73 - equal( title.getMain(), 'Foo_bar.jpg' );
74 - equal( title.getMainText(), 'Foo bar.jpg' );
75 - equal( title.getNameText(), 'Foo bar' );
76 - equal( title.toString(), 'File:Foo_bar.jpg' );
 73+ title = new mw.Title( 'File:quux pif.jpg' );
 74+ equal( title.getName(), 'Quux_pif' );
 75+
 76+ title = new mw.Title( 'File:Glarg_foo_glang.jpg' );
 77+ equal( title.getNameText(), 'Glarg foo glang' );
7778 });
7879
79 -test( 'Transform between Text to Db', function() {
80 - expect(6);
 80+test( 'Main text for filename', function() {
 81+ expect(8);
8182 _titleConfig();
8283
8384 var title = new mw.Title( 'File:foo_bar.JPG' );
84 - title.setName( 'quux pif' );
8585
86 - equal( title.getMain(), 'Quux_pif.jpg' );
87 - equal( title.getMainText(), 'Quux pif.jpg' );
88 - equal( title.getNameText(), 'Quux pif' );
89 - equal( title.toString(), 'File:Quux_pif.jpg' );
90 -
91 - title.setName( 'glarg_foo_glang' );
92 -
93 - equal( title.toString(), 'File:Glarg_foo_glang.jpg' );
94 - equal( title.getMainText(), 'Glarg foo glang.jpg' );
 86+ equal( title.getNamespaceId(), 6 );
 87+ equal( title.getNamespacePrefix(), 'File:' );
 88+ equal( title.getName(), 'Foo_bar' );
 89+ equal( title.getNameText(), 'Foo bar' );
 90+ equal( title.getMain(), 'Foo_bar.jpg' );
 91+ equal( title.getMainText(), 'Foo bar.jpg' );
 92+ equal( title.getExtension(), 'jpg' );
 93+ equal( title.getDotExtension(), '.jpg' );
9594 });
9695
97 -test( 'Initiate from name and set namespace', function() {
98 - expect(1);
99 - _titleConfig();
100 -
101 - var title = new mw.Title( 'catalonian_penguins.PNG' );
102 - title.setNamespace( 'file' );
103 - equal( title.toString(), 'File:Catalonian_penguins.png' );
104 -});
105 -
10696 test( 'Namespace detection and conversion', function() {
107 - expect(7);
 97+ expect(6);
10898 _titleConfig();
10999
110100 var title;
111101
112 - title = new mw.Title( 'something.PDF' );
113 - title.setNamespace( 'file' );
 102+ title = new mw.Title( 'something.PDF', 6 );
114103 equal( title.toString(), 'File:Something.pdf' );
115104
116 - title = new mw.Title( 'NeilK' );
117 - title.setNamespace( 'user_talk' );
 105+ title = new mw.Title( 'NeilK', 3 );
118106 equal( title.toString(), 'User_talk:NeilK' );
119107 equal( title.toText(), 'User talk:NeilK' );
120108
121 - title = new mw.Title( 'Frobisher' );
122 - title.setNamespaceById( 100 );
 109+ title = new mw.Title( 'Frobisher', 100 );
123110 equal( title.toString(), 'Penguins:Frobisher' );
124111
125 - title = new mw.Title( 'flightless_yet_cute.jpg' );
126 - title.setNamespace( 'antarctic_waterfowl' );
 112+ title = new mw.Title( 'antarctic_waterfowl:flightless_yet_cute.jpg' );
127113 equal( title.toString(), 'Penguins:Flightless_yet_cute.jpg' );
128114
129 - title = new mw.Title( 'flightless_yet_cute.jpg' );
130 - title.setNamespace( 'Penguins' );
 115+ title = new mw.Title( 'Penguins:flightless_yet_cute.jpg' );
131116 equal( title.toString(), 'Penguins:Flightless_yet_cute.jpg' );
132 -
133 - title = new mw.Title( 'flightless_yet_cute.jpg' );
134 - raises( function() {
135 - title.setNamespace( 'Entirely Unknown' );
136 - });
137117 });
138118
139119 test( 'Case-sensivity', function() {
——@@ -157,6 +137,16 @@
158138 equal( title.toString(), 'User:John', '$wgCapitalLinks=false: User namespace is insensitive, first-letter becomes uppercase' );
159139 });
160140
 141+test( 'toString / toText', function() {
 142+ expect(2);
 143+ _titleConfig();
 144+
 145+ var title = new mw.Title( 'Some random page' );
 146+
 147+ equal( title.toString(), title.getPrefixedDb() );
 148+ equal( title.toText(), title.getPrefixedText() );
 149+});
 150+
161151 test( 'Exists', function() {
162152 expect(3);
163153 _titleConfig();
Index: trunk/phase3/resources/mediawiki/mediawiki.Title.js
——@@ -7,7 +7,7 @@
88 *
99 * Relies on: mw.config (wgFormattedNamespaces, wgNamespaceIds, wgCaseSensitiveNamespaces), mw.util.wikiGetlink
1010 */
11 -(function( $ ) {
 11+( function( $ ) {
1212
1313 /* Local space */
1414
——@@ -26,15 +26,10 @@
2727 this._ext = null; // extension
2828
2929 if ( arguments.length === 2 ) {
30 - this.setNameAndExtension( title ).setNamespaceById( namespace );
 30+ setNameAndExtension( this, title );
 31+ this._ns = fixNsId( namespace );
3132 } else if ( arguments.length === 1 ) {
32 - // If title is like "Blabla: Hello" ignore exception by setNamespace(),
33 - // and instead assume NS_MAIN and keep prefix
34 - try {
35 - this.setAll( title );
36 - } catch(e) {
37 - this.setNameAndExtension( title );
38 - }
 33+ setAll( this, title );
3934 }
4035 return this;
4136 },
——@@ -63,7 +58,100 @@
6459 } else {
6560 return '';
6661 }
 62+ },
 63+
 64+ /**
 65+ * Sanatize name.
 66+ */
 67+ fixName = function( s ) {
 68+ return clean( $.trim( s ) );
 69+ },
 70+
 71+ /**
 72+ * Sanatize name.
 73+ */
 74+ fixExt = function( s ) {
 75+ return clean( s.toLowerCase() );
 76+ },
 77+
 78+ /**
 79+ * Sanatize namespace id.
 80+ * @param id {Number} Namespace id.
 81+ * @return {Number|Boolean} The id as-is or boolean false if invalid.
 82+ */
 83+ fixNsId = function( id ) {
 84+ // wgFormattedNamespaces is an object of *string* key-vals (ie. arr["0"] not arr[0] )
 85+ var ns = mw.config.get( 'wgFormattedNamespaces' )[id.toString()];
 86+
 87+ // Check only undefined (may be false-y, such as '' (main namespace) ).
 88+ if ( ns === undefined ) {
 89+ return false;
 90+ } else {
 91+ return Number( id );
 92+ }
 93+ },
 94+
 95+ /**
 96+ * Get namespace id from namespace name by any known namespace/id pair (localized, canonical or alias).
 97+ *
 98+ * @example On a German wiki this would return 6 for any of 'File', 'Datei', 'Image' or even 'Bild'.
 99+ * @param ns {String} Namespace name (case insensitive, leading/trailing space ignored).
 100+ * @return {Number|Boolean} Namespace id or boolean false if unrecognized.
 101+ */
 102+ getNsIdByName = function( ns ) {
 103+ // toLowerCase throws exception on null/undefined. Return early.
 104+ if ( ns == null ) {
 105+ return false;
 106+ }
 107+ ns = clean( $.trim( ns.toLowerCase() ) ); // Normalize
 108+ var id = mw.config.get( 'wgNamespaceIds' )[ns];
 109+ if ( id === undefined ) {
 110+ mw.log( 'mw.Title: Unrecognized namespace: ' + ns );
 111+ return false;
 112+ }
 113+ return fixNsId( id );
 114+ },
 115+
 116+ /**
 117+ * Helper to extract namespace, name and extension from a string.
 118+ *
 119+ * @param title {mw.Title}
 120+ * @param raw {String}
 121+ * @return {mw.Title}
 122+ */
 123+ setAll = function( title, s ) {
 124+ var matches = s.match( /^(?:([^:]+):)?(.*?)(?:\.(\w{1,5}))?$/ );
 125+ ns_match = getNsIdByName( matches[1] );
 126+ if ( matches.length && ns_match ) {
 127+ if ( matches[1] ) { title._ns = ns_match; }
 128+ if ( matches[2] ) { title._name = fixName( matches[2] ); }
 129+ if ( matches[3] ) { title._ext = fixExt( matches[3] ); }
 130+ } else {
 131+ // Consistency with MediaWiki: Unknown namespace > fallback to main namespace.
 132+ title._ns = 0;
 133+ setNameAndExtension( title, s );
 134+ }
 135+ return title;
 136+ },
 137+
 138+ /**
 139+ * Helper to extract name and extension from a string.
 140+ *
 141+ * @param title {mw.Title}
 142+ * @param raw {String}
 143+ * @return {mw.Title}
 144+ */
 145+ setNameAndExtension = function( title, raw ) {
 146+ var matches = raw.match( /^(?:)?(.*?)(?:\.(\w{1,5}))?$/ );
 147+ if ( matches.length ) {
 148+ if ( matches[1] ) { title._name = fixName( matches[1] ); }
 149+ if ( matches[2] ) { title._ext = fixExt( matches[2] ); }
 150+ } else {
 151+ throw new Error( 'mw.Title: Could not parse title "' + raw + '"' );
 152+ }
 153+ return title;
67154 };
 155+
68156
69157 /* Static space */
70158
——@@ -118,39 +206,8 @@
119207
120208 var fn = {
121209 constructor: Title,
122 - /**
123 - * @param id {Number} Canonical namespace id.
124 - * @return {mw.Title} this
125 - */
126 - setNamespaceById: function( id ) {
127 - // wgFormattedNamespaces is an object of *string* key-vals,
128 - var ns = mw.config.get( 'wgFormattedNamespaces' )[id.toString()];
129210
130 - // Cannot cast to boolean, ns may be '' (main namespace)
131 - if ( ns === undefined ) {
132 - this._ns = false;
133 - } else {
134 - this._ns = Number( id );
135 - }
136 - return this;
137 - },
138 -
139211 /**
140 - * Set namespace by any known namespace/id pair (localized, canonical or alias)
141 - * On a German wiki this could be 'File', 'Datei', 'Image' or even 'Bild' for NS_FILE.
142 - * @param ns {String} A namespace name (case insensitive, space insensitive)
143 - * @return {mw.Title} this
144 - */
145 - setNamespace: function( ns ) {
146 - ns = clean( $.trim( ns.toLowerCase() ) ); // Normalize
147 - var id = mw.config.get( 'wgNamespaceIds' )[ns];
148 - if ( id === undefined ) {
149 - throw new Error( 'mw.Title: Unrecognized canonical namespace: ' + ns );
150 - }
151 - return this.setNamespaceById( id );
152 - },
153 -
154 - /**
155212 * Get the namespace number.
156213 * @return {Number}
157214 */
——@@ -168,16 +225,6 @@
169226 },
170227
171228 /**
172 - * Set the "name" portion, removing illegal characters.
173 - * @param s {String} Page name (without namespace prefix)
174 - * @return {mw.Title} this
175 - */
176 - setName: function( s ) {
177 - this._name = clean( $.trim( s ) );
178 - return this;
179 - },
180 -
181 - /**
182229 * The name, like "Foo_bar"
183230 * @return {String}
184231 */
——@@ -230,16 +277,6 @@
231278 },
232279
233280 /**
234 - * Set the "extension" portion, removing illegal characters.
235 - * @param s {String}
236 - * @return {mw.Title} this
237 - */
238 - setExtension: function( s ) {
239 - this._ext = clean( s.toLowerCase() );
240 - return this;
241 - },
242 -
243 - /**
244281 * Get the extension (returns null if there was none)
245282 * @return {String|null} extension
246283 */
——@@ -256,37 +293,6 @@
257294 },
258295
259296 /**
260 - * @param s {String}
261 - * @return {mw.Title} this
262 - */
263 - setAll: function( s ) {
264 - var matches = s.match( /^(?:([^:]+):)?(.*?)(?:\.(\w{1,5}))?$/ );
265 - if ( matches.length ) {
266 - if ( matches[1] ) { this.setNamespace( matches[1] ); }
267 - if ( matches[2] ) { this.setName( matches[2] ); }
268 - if ( matches[3] ) { this.setExtension( matches[3] ); }
269 - } else {
270 - throw new Error( 'mw.Title: Could not parse title "' + s + '"' );
271 - }
272 - return this;
273 - },
274 -
275 - /**
276 - * @param s {String}
277 - * @return {mw.Title} this
278 - */
279 - setNameAndExtension: function( s ) {
280 - var matches = s.match( /^(?:)?(.*?)(?:\.(\w{1,5}))?$/ );
281 - if ( matches.length ) {
282 - if ( matches[1] ) { this.setName( matches[1] ); }
283 - if ( matches[2] ) { this.setExtension( matches[2] ); }
284 - } else {
285 - throw new Error( 'mw.Title: Could not parse title "' + s + '"' );
286 - }
287 - return this;
288 - },
289 -
290 - /**
291297 * Return the URL to this title
292298 * @return {String}
293299 */

Follow-up revisions

RevisionCommit summaryAuthorDate
r93826Correct typo in function documentation....krinkle13:46, 3 August 2011
r100110Qunit coverage for mw.Title.js...hashar09:01, 18 October 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r90331Implement mw.Title in core...krinkle09:17, 18 June 2011

Comments

#Comment by Nikerabbit (talk | contribs)   12:10, 2 August 2011

s/sanatize/sanitize/?

#Comment by Hashar (talk | contribs)   15:43, 17 October 2011

Looks like you removed test that create a title given a "name" and a namespace "string": 

-	title = new mw.Title( 'NeilK' );
-	title.setNamespace( 'user_talk' );
+	title = new mw.Title( 'NeilK', 3 );

The test suite does not seem to test the following code: 

 title = new mw.Title( 'NeilK', 'user_talk' );
#Comment by Krinkle (talk | contribs)   17:36, 17 October 2011

new mw.Title( 'NeilK', 'user_talk' );

Never did and still doesn't work, second argument to constructor must be namespace ID. setNamespace has been removed as Title should be immutable. So this case is no longer up for testing.


new mw.Title( 'user_talk:NeilK' ); should work though, do we test for that scenario ?

#Comment by Hashar (talk | contribs)   09:01, 18 October 2011

That last snippet was more or less covered. I have added another test with r100110.

#Comment by Hashar (talk | contribs)   15:44, 17 October 2011

Additionally, do we have some kind of constants in javascript for namespace? Something like NS_MAIN / NS_TALK. I am pretty sure JS does not support constants though.

#Comment by Krinkle (talk | contribs)   17:37, 17 October 2011

No, there are no global constants. We do have wgNamespaceIds though. 


> wgNamespaceIds.user_talk
3

> wgNamespaceIds.image
6

> wgNamespaceIds.file
6
#Comment by Hashar (talk | contribs)   09:02, 18 October 2011

That should be enough. Thanks for the tip :-)

Status & tagging log