User "Brion VIBBER" posted a comment on MediaWiki.r89082. Full URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/89082#c17401 Commit summary:
Adding or adjusting front-end function and variable documentation per our conventions. Also did some trivial clean up, JSHint fixes and performance optimizations while at it. (per http://www.mediawiki.org/wiki/JavaScript_performance ) * Missing semicolons. * [mediawiki.js Line 540] Unreachable 'registry' after 'throw'. * [mediawiki.log.js] Checked for window.console but used console. * Added callback option to mw.util.toggleToc (forwarded to jQuery.fn.slideUp/Down) * Made mw.log argument more descriptive (logmsg instead of string) * Using deepEqual(, true) instead of ok() when asserting true. (ok() is like casting a boolean (or a plain if() statement) to verify that something is not falsy) Added missing test suites for: * mw.util.toggleToc (follow-up r88732, Dummy check was introduced in r87898) /me has been infected by Reedy and will be doing more of this tonight. Comment: So for an example rather than this: <pre> deepEqual( conf.exists( 'foo' ), true, 'Map.exists() returns boolean true if a key exists' ); deepEqual( conf.exists( 'notExist' ), false, 'Map.exists() returns boolean false if a key does not exists' ); deepEqual( conf.get() === conf.values, true, 'Map.get() returns the entire values object by reference (if called without arguments)' ); </pre> I'd consider this: <pre> deepEqual( conf.exists( 'foo' ), true, 'Map.exists() returns boolean true if a key exists' ); deepEqual( conf.exists( 'notExist' ), false, 'Map.exists() returns boolean false if a key does not exists' ); ok( conf.get() === conf.values, 'Map.get() returns the entire values object by reference (if called without arguments)' ); </pre> Comparing a '''variable that should be <code>true</code> or <code>false</code>''' against the expected value directly sounds perfect, and the comments already clarify that the return type is something being checked on the conf.exists() calls. When we're just checking the return value of an === comparison, there's no such need; using <code>ok()</code> lets a maintainer see here you're confirming that a condition is as expected. You might also be able to use <code>[http://docs.jquery.com/QUnit/strictEqual#actualexpectedmessage strictEqual()]</code> here and have it report the differing data if it ever returns the wrong thing, but the docs are a bit vague on what's actually a same-object check behind the scenes. _______________________________________________ MediaWiki-CodeReview mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
