r104990 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r104989‎ | r104990 | r104991 >
Date:19:22, 2 December 2011
Author:danwe
Status:deferred (Comments)
Tags:
Comment:
description message name corrected and minor formatting.
Modified paths:
  • /trunk/extensions/SimpleFarm/SimpleFarm.i18n.php (modified) (history)
  • /trunk/extensions/SimpleFarm/SimpleFarm.php (modified) (history)
  • /trunk/extensions/SimpleFarm/SimpleFarm_Classes.php (modified) (history)

Diff [purge]

Index: trunk/extensions/SimpleFarm/SimpleFarm_Classes.php
@@ -342,13 +342,14 @@
343343 if( in_array( $address, $member->getAddresses() ) ) {
344344 // if script path is required, then check for it too:
345345 if( $scriptPath !== null ) {
346 - if( trim( $scriptPath ) === $member->getScriptPath() )
 346+ if( trim( $scriptPath ) === $member->getScriptPath() ) {
347347 return $member;
348 - } else {
 348+ }
 349+ }
 350+ else {
349351 return $member;
350352 }
351353 }
352 -
353354 }
354355 return null;
355356 }
Index: trunk/extensions/SimpleFarm/SimpleFarm.i18n.php
@@ -16,12 +16,12 @@
1717 * @author Daniel Werner
1818 */
1919 $messages['en'] = array(
20 - 'parserfun-desc' => 'Simple, yet powerfull wiki farm extension without any fancy configuration pages.',
 20+ 'simplefarm-desc' => 'Simple, yet powerfull wiki farm extension without any fancy configuration pages.',
2121 );
2222
2323 /** German (Deutsch)
2424 * @author Daniel Werner
2525 */
2626 $messages['de'] = array(
27 - 'parserfun-desc' => 'Einfache, jedoch trotzdem mächtige Wiki-Farm-Erweiterung ohne großartige Konfigurations-Seiten.',
 27+ 'simplefarm-desc' => 'Einfache, jedoch trotzdem mächtige Wiki-Farm-Erweiterung ohne großartige Konfigurations-Seiten.',
2828 );
Index: trunk/extensions/SimpleFarm/SimpleFarm.php
@@ -43,7 +43,7 @@
4444 /*
4545 * It's no good, at that stage it is too late to set some global variables like $wgScriptPath
4646 * because some others depend on it and being set at the beginning of setup.php after default
47 - * settings and LocalSettings are loaded.
 47+ * settings and LocalSettings are loaded.
4848 * That's also why we can't rely on global functions including hook an message system!
4949 */
5050 // make sure to initialise farm if not manually in 'LocalSettings.php':

Comments

#Comment by Jack Phoenix (talk | contribs)   01:17, 3 December 2011
-				} else {
+					}
+				}
+				else {

Uh? } else { is the recommended version (and also the one which makes sense, IMHO, as such a simple expression should not take more than one line; most certainly not three lines, like I've sometimes seen some people at other projects do) as per our coding conventions.

#Comment by Danwe (talk | contribs)   02:16, 3 December 2011

Where exactly do the coding conventions speak against this style? In my opinion it serves the readability of the nested 'if' structure in this particular case. I have seen this style in our svn many times before and not just for long or complicated expressions and can see nothing wrong with it really.

#Comment by Aaron Schulz (talk | contribs)   02:44, 3 December 2011

Really? I haven't seen that style often at all.

#Comment by Danwe (talk | contribs)   03:19, 3 December 2011

Really. I have 41 extensions in my extensions directory of my projects mw installation, searching over all files for the regular expression "^[\s]+else(\s*if)?\s*\{" gave 275 matches in 126 files... Didn't even require to download the complete trunk/extensions to make my point here I guess ;) So this is being used by popular extensions such as Semantic MediaWiki, Maps, Rename User, CategoryTree and even in MW core!

#Comment by Johnduhart (talk | contribs)   03:11, 4 December 2011
Semantic MediaWiki, Maps,

We let them do their own thing

Rename User, CategoryTree

Ancient doesn't count

even in MW core

Maybe 5 times out of a few hundreds elses?

#Comment by Danwe (talk | contribs)   05:15, 4 December 2011

"Semantic MediaWiki" isn't in this svn repository, right. But Maps is being developed within this svn actually and its using this style more than just a few times (if I am not mistaken 33 "else(if) {" vs 15 "} else(if) {". Since I have been working on Maps a while ago, It might even be possible that I have adopted that style from there.

IMHO its nothing worth arguing about a lot. It also seems like there is no definite agenda on this. If there is going to be and if other extensions, which are more excessive in using this, are going to change it, then I would be happy to follow as well. Anyway, I see no point in further counting how often this is being used where exactly. You could as well pick as good as any other construct of the language and fight about where to draw a line exactly. MW Coding conventions aren't that strict always, you will see if you take a look at "switch case" or optional spaces behind control structures.

#Comment by Reach Out to the Truth (talk | contribs)   05:58, 4 December 2011

Well, you're the one who changed it and you're the one counting how many instances of your preferred version there are. No one else cares enough to do that.

Though the documentation is off-site, Semantic MediaWiki is in fact in the official repository here. It's at /trunk/extensions/SemanticMediaWiki, and there are also a billion other Semantic* extensions available in the repo if you're interested.

#Comment by Danwe (talk | contribs)   14:14, 4 December 2011

Uh, it really is. So I don't understand why they "do their own thing" then, besides the fact that "their own thing" isn't that different from the rest. And to make it clear, "} else {" still is my preferred style but there might be exceptions in favor of readability from time to time when I feel like it ;)

#Comment by Reach Out to the Truth (talk | contribs)   05:05, 4 December 2011

People may be using a coding different style for other projects, and I don't think stylize.php enforces MediaWiki's preferred else style, so there are some deviations from this style. But stylize.php uses it internally, which in combination with what it says on the coding conventions page is surely enough evidence that } else { is the normal convention in this project.

Status & tagging log