Fix Drupal's awkward coding standards for the . operator

Unfortunately, I'm not in a position now where I can actually test a patch but I want to put my wholehearted endorsement behind the idea. The lack of space before the dot drives me crazy and I always ignore coder when it complains about my space. I follow most of Drupal's conventions but that one has never made any sense to me. If you're going to put a space after, which I agree makes it easier to read, why on Earth would you not put one before?

+1 from me on the idea for sure!

Michelle

My personal belief is that the . operator has an exception in its coding standard rules that doesn't make any sense. My best guess is that because '.' is also a decimal point, some people feel it 'reads' better to have it next to strings. However, I disagree with this; I find it much more awkward to have a single operator that has an exception that is not adequately explained.

In my personal code -- i.e, all of my modules, this is the one coding standard I refuse to participate in. Now, bear in mind that I come from a background of C++, camelCase and various other differences in Drupal's coding style. For the most part, though, Drupal's coding style is sensible, well explained, and easy to follow. This one inexplicable exception, however, makes the perfectionist in me just want to scream.

I am fully behind this effort.

This patch fixes some indentation issues with aligned comments which got shifted to the right. It also does not include changes to the files in the scripts/ folder anymore.

+1 from me, but it's ignorable as I have no intention of testing the patch ;)

+1 to the idea.

It looks like you missed the ones that have a string on each side of the dot, and hence no space at all. Example:

+1 on this patch.

It never made sense to me why it has an awkward rule. More spaces means better readability.

I applied it to today's checkout to HEAD and it did not break anything.

@John Morahan: Yep, thanks for noticing. I changed them now.

For reference, the string concatenation coding standard is documented in http://drupal.org/node/30785.

Interesting. So those dots with no space on either side shouldn't have been there even under the current rules. I did not know that.

The one rationale I could think of (after this standard forced me to think of one) has been it can be slightly easier to glance at a mess of concat'ed PHP and HTML and see (at a glance) which sections of a line are code and which are strings. mostly just in the case of something like a template which actually has a lot of concats and quotes on one line. but, hard to justify if no other major projects have such a standard.

The // doesn't look like it belongs in this patch.

mfb saved me some writing: I also think that that is the reason for this Drupal-specific coding standard. That said, I'd prefer this to be the same as in virtually every language, so you have my +1.

Ugh. Back to CNW

I think most people nowadays use editors with syntax highlighting that makes strings easy enough to recognize without this unusual convention.

There are still a few dots without spaces at the ends of lines. Example:

File me as +1 on this as well. Given the size of the patch, if there is a consensus about it we could commit it and then find the stragglers later, after updating coder module to find them for us. :-)

Sure. I just thought I might mention them, since kkaefer seems to have some quick means of fixing them at his disposal ;) But don't let me hold this up.

@John: thanks for pointing out these issues. The comment got into the patch as I tested some regexes, but I forgot to remove it later on.

Well, I can't find any more problems.

Applies cleanly. I support this patch.

kkaeffer, you probably used a regular expression to make these changes. It's not hard to write it, but it might be good to have a standard one that could be used by others to update contrib modules. Can you share the one you used?

+1 for me. Please make sure douggreen is on board for the Coder module. And there is no reason why Coder has to wait for D7. Let's do it now! (As a matter of fact, I would say that all bug fixes for D5 and D6 should begin doing it this way.)

I don't think that it's a good idea to apply that to D5/6 as well since it makes things inconsistent.

@flobruit: I used a variety of different regular expressions in TextMate (so no snazzy shell scripts here) which can easily be written in a couple of minutes.

It seems I'm the only one who would vote -1 on this. Am I?
I always liked this rule, because it concatenates strings and vars optically. XXXXXX. xxxx .XXXXXX needs less space and somehow looks less displaced than XXXXXX . xxxx . XXXXXX for me.

However, I don't think this question is important. The coder_format script can be changed accordingly, so updating (at least my) modules should be done within seconds.

-1 i actually like the current convention... i don't really see much to gain from switching it.

I'm with drewish on this. I think the extra space makes lines longer and isn't really necessary as the most common thing to do with strings on a line with other operations is concatenate them. -1 here.

No offense but "I like the convention" doesn't seem to be a very good defense as to why to make an exception. "Makes lines longer" is the same defense people who prefer no spaces around operators at all give; I disagree with it. And the most common operation with numbers is to add/subtract/assign them, but I don't see anyone advocating to remove spaces there.

I would really love to see an honest justification for this convention but none of the ones presented here make sense to me.

And the answer for what is to be gained is "consistency". Coding styles are primarily about consistency.

To anyone who's -1 to this change, I challenge you to the following: Count how many sentences it takes to explain to another non-Drupal developer how to use the current standard. ;) My personal best is 4. ;)

I do personally prefer the current standard now that I have my head around it, but this is a consistent "wtf?" for new developers, and furthermore it's consistent with existing PHP standards. +1.

Oh, sun's visual example in #24 is misleading; the use of capitalization makes a big difference. But what if I gave this example:

XXXXX+ xxxx +XXXXX

Should we not treat the addition operator the same as the concatenation operator?

And then there's always "++$count" and "$count++". Let's not even get into the combinations with the equal sign... ;-)

It will probably come as a shock to Earl that I'm agreeing with him. And I don't know that kkaefer wants to roll another 800K patch to remove spaces from the arithmetic operators.

I think the bottom line on this one is which way does Dries want it?

$count++ and ++$count make sense because they are unary operators; they work only on the variable they are referencing. It's actually very important not to have the space there.

I can't think of any place in the coding standard that suggests no spaces for assignment operators; in general, no-space on operators is reserved for unary operators, except for concatenation, which has its spacing determined by rvalue type.

webchick, I can explain in two sentences! :) But really, this is a good change, I am for it. Next issue should be the coding standards for cast. Not this one, though.

I disagree, Merlin. His example is not misleading because it's precisely because of the number, shape and location so pixels used on screen AND the printed conventions we all grew up with regarding numerals, that this rule makes some sense to some (include me with sun as far as personal preferences go).

That is, it LOOKS clearer and better (at least, to some of us). And contrary to the argument that it's just something we "like" -- clarity and ease of reading are important and rightfully influence coding standards.

That said, I don't care much if the dot concatenate standard is changed, as there are other parts of the Drupal coding standards which irritate me a lot more and are unlikely to ever change. That's the beauty of coding standards -- they annoy everybody just about equally, but we use them for good reason. :-)

+1 from me. For consistency, and because it regularly catches me out and seems unnatural when I have to go back to fix it.

I disagree that the current format "looks cleaner and better". The period is too easy to miss in many cases. And, as others have noted, it's also inconsistent with every other binary operator.

That's just plain weird. Either we change . to have a space on both sides, or we change + to only sometimes have space padding. I've never actually seen a standard of "except next to a literal" on space padding in any language/project. Let's be consistent and put spaces in both places.

+1 for making the concat coding standards consistent with other operators and with other PHP projects. But…

4 out of 22 hunks FAILED -- saving rejects to file modules/filter/filter.module.rej

Re-rolled.

If it's rerolled to be retried, shouldn't it be PCNR? Has anyone asked Dries about his feelings on this? After all, he gets a ±1,000,000 vote from Mt. Olympus. ;-)

Missed the new function filter_update_7001().

Removes a wrong replacement in node.module.

Chris:

I disagree, Merlin. His example is not misleading because it's precisely because of the number, shape and location so pixels used on screen AND the printed conventions we all grew up with regarding numerals, that this rule makes some sense to some (include me with sun as far as personal preferences go).

You say numerals, and I can agree with that. +XXX is often used for numerals. THe problem is, his example is being used for strings and variables.

Does the above really appear equivalent?

(I can barely *type* the standard, as I have to think about where I put the dot, whereas almost all of the other coding standards are reduced to muscle memory).

Personally, whenever I see the . up against the string, my first though is "How can a string have a decimal?"

Most of those who have voted -1 on this change have stated that it's not important if this standard would change. Also, please see my note about coder_format in #24.

We should stop arguing against or for this change, and also hold off further patches, until Gabor and Dries have spoken.

Let's invest our time and energy in more valuable things until that happens. Marking as RTBC to get some attention.

Actually, Gabor is the Drupal 6 maintainer and doesn't have any say in Drupal 7; the Drupal 7 maintainer has not been selected at this time and likely will not be for at least a couple more months.

So it's just Dries.

Committed to HEAD. I've spent some thinking about this and comparing the different examples, and I agree that the proposed changes lead to improved consistency. Thanks all.

Just to follow up…

The coding standards page has been updated: http://drupal.org/node/30785

Assuming coder module does/could warn about this, I added a feature request in that issue queue: #246568: Coding standards of string concatenation have changed

Should this also apply to javascript concatenation (the + operator)?

Committed to HEAD. I agree that this is more consistent.

In #246568: Coding standards of string concatenation have changed the question arose, whether this change in Drupal's coding standards may be already applied to contrib modules and Coder module for D6 (and below). I think that authors of contrib modules can already change their coding style. However, if the automated testbed for Drupal core patches also checks the coding style, Coder module must not be updated prior a release of Coder module compatible to D7.

IMO it should be changed for D7 and above... else Coder's going to barf errors on a D6 core install unless we roll a similar patch for D6, and I'm less supportive of such an invasive change in a stable release.

Changing D6 core like this, absolutely not. Changing Coder so that it's more permissive and allows either version for D6, I'm OK with. (I've already started using the extra space for my own code, Coder module or not :-).)

I don't suppose anyone has considered a setting?

I agree we should not backport this code styling to Drupal 6 core. There's too much change and no bug fixes in this patch.

Any new code in contrib should follow the new style guide starting from now, IMO. Even if they are writing a new module for Drupal 4.6. (*shudder.*) And old code can get updated as needed.

As for the Coder module (settings, etc.), shouldn't we be discussing that over there? --> #246568: Coding standards of string concatenation have changed

Please tell me this is a bad dream.... I am incredibly -1 to this. HUGE -1. Here's my rational argument:

1) Having dots next to quotations increases readability by having the eye treat the entire thing as one chunk: 'foo'. $foo makes the concatenation easier to read.

2) This also makes people think twice before concatenating two variables directly. For example: $module . $hook spaces them out so much that something looks wrong. This is very good, as something is usually wrong with such attempts; something like $module .'_'. $hook would make more sense, and even looks better. With the new patch, $module . '_' . $hook looks equally bad.

3) The rule is not really that hard to learn. It's even very very simple. Space between dots and anything besides quotes.

4) This just looks wrong to me now. It just looks unDrupal, and thus unawesome by correlation.

5) It serves to separate variable strings more from the hard coded strings. For example: 'This is as '. variable_get('awesome', 'Awesome') .' as possible!'. This makes it clear that you're sticking variable stuff directly into the string. On the contrary, if we had something like 'This is as ' . variable_get('unawesome', 'Unawesome') . ' as possible!' does not do an excellent job of keeping the variables and the strings separate at first sight.

6) Having the dots in the right places is like music; it makes the code simply sing. :) Having the dots in the places of the new coding standards is like garbage; it makes the code stink.

7) The new stuff looks way too spaced out. If I have something like: 'This '. $animal .'\'s '. $property .' is '. $value, it just looks really messed up with the new coding standards: 'This ' . $animal . '\'s ' . $property . ' is ' . $value.

8) This was committed within a five day period and there will likely be other cries of outrage like this once people realize what terrible crimes of unawesomeness you've committed.

9) It gives me an easy excuse to not actually review patches.

10) Concatenating string to string just looked wrong under the old conventions: 'This '.'thing'. Under the new coding standards, however, if you originally had something like 'This ' . $adjective . ' thing' and decide to remove $adjective, alarm bells do not immediately ring "I've done something wrong!" 'This ' . ' thing' looks fine at first glance.

11) No one will know that the coding standards have changed, and will likely continue to code their string concatenations the way they were originally taught to. If you're going to keep this, at least publicize it. Maybe a front page post saying, "Hey, look what we've done to deawesomify Drupal!".

12) Please?

13) Pretty please?

14) We don't want Drupal to lose its identity. Just because other people are doing things other ways doesn't make those ways right and our ways wrong. We should be proud to express our awesome string concatenations!

15) I just don't like the new conventions.

16) The new conventions make code look ugly.

17) The new conventions make flowers shrivel up and die alone in a corner.

18) Why don't we create a poll and see how many people actually think the standards should be changed, rather than having a few people discuss it in a sheltered irc channel or a few more discuss it on a hard-to-find issue in the queue.

19) It is relatively easy to roll this back now. Wait and it may not be!

20) I think that's about it. I'll post a follow up if I think of any more reasons.

The title is left because the new coding standards are even MORE awkward. Please, please rollback!

#1: I totally disagree. If it really made it easier to read, I would've gotten used to it by now. IMO, it does not make it easier to read, it makes it harder to read. I admit there is a certain amount of "beauty in the eye of the beholder" going on here, so I'm willing to leave this neutral: when added together, I think readability is equivalent; I have more trouble reading it the old way, cwgordon07 has more trouble reading it the new way.

#2: "Just looks wrong". Beauty is in the eye of the beholder. $module .'_'. $hook is the one that looks wrong to me. This is essentially argument #1.

#3: The rule is 'easy to learn' but it's an exception and exceptions are inconsistent and must be justified.

#4: Repeat of #2.

#5: I don't follow. It's mostly a repeat of #2.

#6: I agree with #6, except my conclusion is different. Having an exception to the operator rule that isn't justified is like hitting a flat note every time we concatenate.

#7: Repeat of #2. Also, if things are way too spaced out, we should eliminate spaces around all operators.

#8: terrible crimes of unawesomeness. Um. Kay.

#9: Okay.

#10: There are lots and lots of errors that can be left over from changes and I seriously doubt this particular one comes up often enough to justify this.

#11: I stop here because the attitude is pissing me off.

"18) Why don't we create a poll and see how many people actually think the standards should be changed, rather than having a few people discuss it in a sheltered irc channel or a few more discuss it on a hard-to-find issue in the queue."

It was on the dev list and in the issue queue in addition to IRC. The people that need to be worried about this should be reading at least one of those places. Where do you propose it get discussed? On the front page? That'll really baffle the average user coming here to check out this CMS...

Michelle

#9 Hmm, don't read them, then. Just test them.

This patch was never intended to be applied to Drupal before 7.x. It should not be applied to any code that is related with Drupal before 7.x.

Automatically closed -- issue fixed for two weeks with no activity.

just to let cwgordon7 and add1sun know they're not alone...

I totally agree with cwgordon7...

in addition

1) this change is totally superfluous does absolutely nothing to improve the drupal project, besides confuse developers already engaged in the project.

2) Is a total waste of every reviewers time who could have been reviewing a real functional patch.

3) totally wastes all previous training to the prior coding style.

I can't believe you all wasted your time to get a patch/change like this committed, over a totally subjective matter that should never have even arisen, and should have been moot under the purview of prior coding standards... how many lines of code got touched for a zero impact change that only added more whitespace to our code.

LAME

ya -1.

this is lame. waste of time. so many better tickets that could be given the attention.

kill the ticket now. :S

I wouldn't mind either if this was reverted. However, it seems that most of us agree on this change. Sometimes, we need to accept that Drupal is not only about do-ocracy, but also democracy.

this change is totally superfluous does absolutely nothing to improve the drupal project, besides confuse developers already engaged in the project

It improves the Drupal project by eliminating confusion of new developers. It also eliminates confusion by many existing developers (see the many previous comments.) It does so at the expense at some existing developers. :-( But you guys are smart, I’ll know you’ll adapt. :-)

The only rationale I can see for the old standard is kerning: I know source code should be readable, but typography is just going too far; and in a monospace font too!