As a follow-up to #2687941-36: [Policy, no patch] Within the Function Calls section, delete explicit mention of padding spacing in a block of related assignments, I propose changing the "Operators" section in the following way. Instead of:

All binary operators (operators that come between two values), such as +, -, =, !=, ==, >, etc. should have a space before and after the operator, for readability.

put this:

All binary operators (operators that come between two values), such as +, -, =, !=, ==, >, etc. MUST have exactly one space before and after the operator, for readability.

This would explicitly forbid the use of more than one space, for instance for lining up subsequent assignment (as was explicitly permitted before #2687941: [Policy, no patch] Within the Function Calls section, delete explicit mention of padding spacing in a block of related assignments). The arguments for and against this change can be found in the other issue. In short: while it might make code a bit more readable, it also adds a chance for unnecessary merge conflicts due to the changes to unrelated code that adding a new statement/expression might make necessary in such cases.

(Note that I also changed the example operators to have individual <code> tags around them. I'd argue that's the correct way to do it – but that's of course not the main point of this issue.)

Comments

drunken monkey created an issue.

pfrenssen’s picture

(Note that I also changed the example operators to have individual <code> tags around them. I'd argue that's the correct way to do it – but that's of course not the main point of this issue.)

Not the main point of the issue indeed, but thanks for the suggestion! I have updated the formatting of these operators.

Regarding this issue, we can start by rounding up the comments from #2687941: [Policy, no patch] Within the Function Calls section, delete explicit mention of padding spacing in a block of related assignments of the people that were in favor or against single spacing. The impression we had when this was previously discussed is that there is a deep division between people that either really want to have multiple spaces, or are absolutely against it.

I personally am in the single space camp, this is how the majority of core does it at the moment. A very rough grep of core comes up with only around 460 out of 230000 cases where multiple spaces are used for the assignment operator, and most of these are clustered in a handful of files. Counted in a percentage this means that 99.8% of core is currently following the single space format.

Morbus Iff’s picture

I was one of the folks in the "multiple spaces" camp.

Counted in a percentage this means that 99.8% of core is currently following the single space format.

This is confirmation bias and wrongly plotted, in my opinion. No one in the multiple space camp is saying that multiple spaces should be used ALL THE TIME for every assignment operator. We're ONLY saying that, every so often, they can improve readability. The statistic above is a bit misleading in that it's (probably?) measuring SINGLE assignment expressions alongside MULTIPLE assignment expressions. Most of the time, code usually follows this pattern:

$assign = 1;

then a control structure

The following is a much rarer case:

$assign  = 1;
$example = 2;

then a control structure

The other times I use multiple spaces, and always on a case-by-case basis (never "ALL THE TIME"), are array key/values and ternaries:

$array = array(
  'one'   => 'two',
  'three' => 'four',
);

$something  = isset($beast) ? $bat       : $human;
$everything = isset($man)   ? $cannibal  : $beatnik; 

Not only is the 99% statistic likely wrong from a plotting perspective, I'd like to hilariously and cynically suggest that it's also wrong from a political perspective: it's like saying that because 99% of humans are heterosexual, than the remaining 1% can't be homosexual. I jest, I jest! (This extreme example was only meant to illustrate my "you'll never convince me with a statistic" feelings).

What I think is more interesting though is that the statistic actually works AGAINST the "single space" folks. The ONLY non-opinion argument offered for why single spaces are "better" than multiple spaces is that they can screw up a diff. But, the statistic indicates that this can only plausibly happen less than 1% of the time, and probably even less than that given the likelihood of two folks touching the same two lines of code in a single patch across multiple issues.

As such, we should be liberal in what we accept, because the argument against isn't very compelling.

pfrenssen’s picture

For me personally the "readability" argument does not make sense. For me spacing out the assignments makes it absolutely less readable. Code is written horizontally not vertically.

There was an example given in the previous thread about this:

  // Colors that are used in testing.
  protected $black       = array(0, 0, 0, 0);
  protected $red         = array(255, 0, 0, 0);
  protected $green       = array(0, 255, 0, 0);
  protected $blue        = array(0, 0, 255, 0);
  protected $yellow      = array(255, 255, 0, 0);
  protected $white       = array(255, 255, 255, 0);
  protected $transparent = array(0, 0, 0, 127);

This for me makes it harder to read. I have trouble "connecting" the left and right sides of the assignment.

The same goes for this example:

$something  = isset($beast) ? $bat       : $human;
$everything = isset($man)   ? $cannibal  : $beatnik; 

Because of the vertical grouping in my mind this reads like "beast - man", "bat - cannibal", "human - beatnik", and not like it's intended.

Morbus Iff’s picture

We'll agree to disagree. And that's OK! ;)

In the color example, I "see" that as two visual and deliberately vertical columns - i.e., the verticality is exactly why these spaces exist. Only the left column is supremely important because "$black" is so clear of a variable name that I never have to look over to the right side column (past the initial writing and testing, of course). So, by splitting the code into two vertical columns, it allows my eye to focus ONLY on the left column - I would never need to "read across", as it were, to the second column. The extra whitespace makes it easier to see the variable names because there is a much greater delineation between the "good stuff" (the variable name) and the "bad stuff" (the CYMK values which, by themselves, mean absolutely nothing). And, if I did need to look at the CMYK, I'd place my cursor on the line I wanted to read, my code editor would color that line differently than the other lines, and I'd be able to clearly "see" only the line I'm interested in (i.e., the one my cursor was on).

As for the ternary, I'll accept it's not a very good example. In real life, I probably wouldn't have whitespaced them. But, for me, the visual and vertical columns here is exactly the goal: what you see as a detriment, I see as the intended positive: I "see" this as a "table/spreadsheet" with invisible headers of "Test", "Positive", and "Negative", and I see the "beast - man" as the conditions, "bat - cannibal" as the positives, and "human - beatnik" as the negatives. It is entirely possible I "see" things like this more easily because I'm so used to formatting in other text-based syntaxes (like Markdown or MediaWiki) where these equivalently vertical "lining up" helps a great deal.

TR’s picture

-1

I completely agree with what @Morbus Iff said in #3.

If the amount of whitespace doesn't affect readability, then why do we MANDATE that code is indented, and why do we MANDATE that nested tags in templates are indented, etc.?

OF COURSE the amount of whitespace affects readability. Let the code author continue to use more than one space if he/she judges that it improves readability.

Note that Drupal Coding Standard have for a LONG TIME explicitly allowed multiple spaces if it increases readability. Changing this standard to forbid multiple spaces in all cases will generate needless work that will not improve code quality in any way.

This affects me because I personally maintain several hundred thousand lines of code in contributed modules, and I sometime use multiple spaces for readability. I would not appreciate having to go through all that line-by-line to make changes that don't to anything to improve my code.

borisson_’s picture

I'm a huge +1 for this as well.

When adding a new, longer line to such a block this can lead to extra whitespaces and harder to understand diffs. This is one of the biggest reasons why I'm +1 on this issue.

tizzo’s picture

This issue was discussed on today's coding standards committee call and we feel that this issue needs more support. We are currently counting three in favor and two opposed and we feel that we need more support before announcing for final discussion.

neclimdul’s picture

In a vote I'm against forbidding > 1 whitespace. I'm for allowing indenting for readability for largely the reasons Morbus laid out.

JvE’s picture

While I do like the readability that multiple spaces sometimes provide, I do not like having to change unrelated lines when adding or removing another line.
Having:

  protected $red    = array(255, 0, 0, 0);
  protected $blue   = array(0, 0, 255, 0);
  protected $orange = array(255, 255, 0, 0);

Add one, change three:

  protected $red         = array(255, 0, 0, 0);
  protected $blue        = array(0, 0, 255, 0);
  protected $orange      = array(255, 255, 0, 0);
  protected $transparent = array(0, 0, 0, 127);

Remove one, change two:

  protected $red  = array(255, 0, 0, 0);
  protected $blue = array(0, 0, 255, 0);

And eventually these things always end up like:

  protected $black = array(0, 0, 0, 0);
  protected $red   = array(255, 0, 0, 0);
  protected $green = array(0, 255, 0, 0);
  protected $transparent = array(0, 0, 0, 127);
  protected $orange = array(255, 255, 0, 0);
  protected $blue   = array(0, 0, 255, 0);
  protected $yellow = array(255, 255, 0, 0);
  protected $white = array(255, 255, 255, 0);

so a +1 from me.

neclimdul’s picture

That's a common complaint. Its possible you do know this but the quick fix is multiple cursors which is available in most editors. I use it most days for a host of other things and just a great tool to have around.
https://www.jetbrains.com/help/phpstorm/multicursor.html
http://www.sublimetext.com/docs/selection
https://daijiang.name/en/2015/04/10/useful-atom-shortcuts/#multi-cursor

quietone’s picture

There is now a sniff for this. Is the documentation changed needed?

Morbus Iff’s picture

Link to sniff?

Morbus Iff’s picture

Oh, you must mean:

https://github.com/squizlabs/PHP_CodeSniffer/wiki/Customisable-Sniff-Pro...

Which isn't so much "Drupal has decided to enforce this ruling" as "Coder happens to use this third-party ruleset." But what's important is the default configuration does not prevent extra spacing before the operator: "A number of coding standards allow multiple assignments to be aligned inside a single code block. When doing this, the number of spaces before an assignment operator can be greater than 1. To support this coding style, this sniff does not check the spacing before assignment operators by default."

And I still align operators occasionally in my own code, and I don't recall it being flagged.

Can you provide more details about the enforcement?