r109786 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r109785‎ | r109786 | r109787 >
Date:01:08, 23 January 2012
Author:krinkle
Status:ok (Comments)
Tags:
Comment:
[ApiSandbox] bug fix
* Looks like API is returning HTML here, got a couple of <br> and <li>'s
* Wrapping parens only if there is a link, got a couple of loose () before
Modified paths:
  • /trunk/extensions/ApiSandbox/ext.apiSandbox.js (modified) (history)

Diff [purge]

Index: trunk/extensions/ApiSandbox/ext.apiSandbox.js
——@@ -363,15 +363,15 @@
364364 */
365365 setHelp: function ( $container ) {
366366 var linkHtml = '',
367 - descTxt = smartEscape( this.info.description );
 367+ descHtml = smartEscape( this.info.description );
368368 if ( this.info.helpurls && this.info.helpurls[0] ) {
369 - descTxt = descTxt.replace( /^([^\r\n\.]*)/, '$1' ) + ' ';
370 - linkHtml = mw.html.element( 'a', {
 369+ descHtml = descHtml.replace( /^([^\r\n\.]*)/, '$1' ) + ' ';
 370+ linkHtml = mw.msg( 'parentheses', mw.html.element( 'a', {
371371 'target': '_blank',
372372 'href': this.info.helpurls[0]
373 - }, mw.msg( 'apisb-docs-more' ) );
 373+ }, mw.msg( 'apisb-docs-more' ) ) );
374374 }
375 - $container.text( descTxt ).append( mw.msg( 'parentheses', linkHtml ) );
 375+ $container.html( descHtml ).append( linkHtml );
376376 },
377377
378378 input: function ( param, name ) {

Comments

#Comment by Kaldari (talk | contribs)   05:23, 27 January 2012

descHtml.replace( /^([^\r\n\.]*)/, '$1' )

I give up. What does this actually do? It looks like it's matching to any set of characters that don't include '\r', '\n', or '.', and then replacing that set of characters with itself. Does that ever actually change the output?

#Comment by Krinkle (talk | contribs)   18:42, 27 January 2012

I don't know. Didn't write it, didn't change it. I'm curious as well.

#Comment by Kaldari (talk | contribs)   19:09, 27 January 2012

Perhaps they're trying to do something like: 

descHtml.replace( /\s*[\r\n\.]+\s*/g, ' ' )

I'm pretty sure the existing code will never do anything.

#Comment by Kaldari (talk | contribs)   19:25, 27 January 2012

It looks like prior to r109726, the regex was used to match to any set of characters that didn't include '\r', '\n', or '.', and then stick that set into an anchor tag. Now, however, it just sticks it back into descHtml. I'll add a fixme note to r109726.

#Comment by Kaldari (talk | contribs)   19:44, 27 January 2012

I'm also confused by the moving of mw.msg( 'parentheses' ) from the .append() to the linkHtml var. I don't think this will actually change anything and it makes the code a bit harder to read.

#Comment by Krinkle (talk | contribs)   20:30, 27 January 2012

No, there is a significant different. A bugfix even.

If linkHtml is not populated (e.g when if ( this.info.helpurls && this.info.helpurls[0] ) { evaluates to false), then calling mw.msg( 'parentheses', linkHtml ) would add a bogus empty pair of parentheses ("()") to the end of the textnode. The movement is a bugfix in this commit.

#Comment by Krinkle (talk | contribs)   20:31, 27 January 2012

No, there is a significant different. A bugfix even. If linkHtml is not populated (e.g when if ( this.info.helpurls && this.info.helpurls[0] ) { evaluates to false), then calling mw.msg( 'parentheses', linkHtml ) would add a bogus empty pair of parentheses ("()") to the end of the textnode. The movement is a bugfix in this commit.

#Comment by Kaldari (talk | contribs)   22:02, 27 January 2012

You're right. I totally overlooked the if! Marking OK.

Status & tagging log