Bring code up to coding standards

You forgot to attach the patch.

If you decide to attach a patch, or want to say which files needed help, please feel free to reopen this issue... Also, you should probably change the component, since if you are changing code it should not go as a "documentation" issue.

I think the component choice is fine, and for this type of patch, one big patch is fine.

The only thing I would suggest is putting it as "bug report" and changing the title.

So I took a quick look at this patch. As you stated above, the main thing it appears to be doing is changing else if to elseif, and != in SQL statements to <>. Have you verified that these changes are consistent with our coding standards?

I noticed another change you made too: adding spaces around some . string concat operators. However, you didn't seem to be consistent/complete about this one. For instance, this change was not made in block.module in the lines you did change, and in blogapi.module, where you added a space before a ., you didn't add one after the .. And since the 6.x code base doesn't generally have spaces around ., which is bad and doesn't agree with current coding standards, it would be a big pain to change all of the instances. So I am not sure about this change... Either change all of them or none of them, please.

And there are some other changes too, such as changing true/false to TRUE/FALSE... again, have you examined the entire 6.x code base for this?

Anyway... All of that aside, it would be better to focus such efforts on 7.x, where we (a) are actively developing and (b) have automated tests so we could test and make sure your patch doesn't break anything... In 6.x, I guess you can download the Simpletest module and run all the tests it come with -- have you done so to make sure your patch is OK?

While I agree that such efforts would be better spent on HEAD (or 7.x right now), they have already been made for 6.x and surely does not do any harm.

Regarding the string concatenation operators, there aren't a lot of these, and they're all in the form of changing ").'foo'" to ") .'foo'", which was the standard pre-7.x.

Whether or not the entire 6.x code base has been checked, these are still sound changes - and they still apply.

+++ modules/system/system.install	31 Oct 2009 08:24:35 -0000
@@ -691,11 +691,11 @@ function system_schema() {
+        'not null' => TRUE, ),

I'm pretty sure the ")," should be on it's own, new, line.

+++ modules/system/system.install	31 Oct 2009 08:24:35 -0000
@@ -691,11 +691,11 @@ function system_schema() {
+        'not null' => TRUE, ),

Ditto.

+++ modules/system/system.install	31 Oct 2009 08:24:35 -0000
@@ -2351,9 +2351,9 @@ function system_update_6043() {
+                  'unique keys' => array('tmd' => array('theme', 'module', 'delta'), ),

Ditto. Even if I'm not sure it's a good idea here, unless we actually "fold out" the array to a proper list, which might just actually be the better code style.

Powered by Dreditor.