Forbid using more than one space around operators

(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.

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.

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.

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.

-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.

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.

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.

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

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.

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

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

Link to sniff?

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?

Convert to new template.

The related sniff is Squiz.WhiteSpace.OperatorSpacing. I enabled that on core and didn't get any violations.

On another issue pfrenssen explained that the Drupal coding standards uses a friendly style and didn't use 'MUST'. Should the wording be changed?

@quietone, yeah, that's the one I mention in #14.

It specifically *allows* more-than-one space BEFORE the operator, as I quoted above.

Running this against core would allow and NOT FLAG the following:

  // Multiple assignment with whitespace beforehand.
  $variable       = 1;
  $variableLonger = 1;

Thus, changing the code style docs to MUST "only 1 whitespace" would mean we have _no lint that checks for it_.

I updated the text and added myself as supporter in the new template.
We should find a sniff that is stricter than the on in#15, for reasons in #16.

The sniff has a property for spacing before the assignment.

Using the code in #16 with this

  <rule ref="Squiz.WhiteSpace.OperatorSpacing">
    <properties>
      <property name="ignoreSpacingBeforeAssignments" value="false" />
    </properties>
  </rule>

reports these errors

---------------------------------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
---------------------------------------------------------------------------------------------------------
 53 | ERROR | [x] Expected 1 space before "=>"; 3 found (Squiz.WhiteSpace.OperatorSpacing.SpacingBefore)
 54 | ERROR | [x] Expected 1 space before "=>"; 2 found (Squiz.WhiteSpace.OperatorSpacing.SpacingBefore)
---------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY

Adding my +1, mostly due to the same reasons in #10

+1 I think it's more readable with just one.

I don't think asymmetrical is really ever more readable in the first place.

Updated issue summary to include nicxvan's support.

Changing to NW for a change record, step 3

-1 from me.

The improvement to readability happens every time a developer looks at the code in question. That's a frequent benefit.

The downside of a merge conflict happens only when that code is changed. That's a rare cost.

For clarification of anyone catching up, Squiz.WhiteSpace.OperatorSpacing is actually in core today. We added it in #3099583: Update coder to 8.3.7 replacing the Coder WhiteSpace checker, we just don't use the property related to this issue.

Not really sure what the supporting user section is for but just also its been years just to be clear, still -1 for stricter rule. +1 for maintaining readability.

Morbus' recent example and the other examples in this thread are succinct for discussion but in reality most use is like the below code from our SQLite driver where the tabular format is useful for comprehension.

$magic_map_or_list = [
  'varchar_ascii:normal' => 'VARCHAR',

  'varchar:normal'       => 'VARCHAR',
  'char:normal'          => 'CHAR',

  'text:tiny'            => 'TEXT',
  'text:small'           => 'TEXT',
  'text:medium'          => 'TEXT',
  'text:big'             => 'TEXT',
  'text:normal'          => 'TEXT',
  // another like 20 lines mapping data types.
];

To joachim's point, these uses are often read and slowly changing.

The thing is I find this much harder to read, you have to carefully ensure you're on the same line to line up the variable assignments. This gets worse if it's longer.

$magic_map_or_list = [
  'varchar_ascii:normal' => 'VARCHAR',

  'varchar:normal'       => 'VARCHAR',
  'char:normal'          => 'CHAR',

  'text:tiny'            => 'TEXT',
  'text:small'           => 'TEXT',
  'text:medium'          => 'TEXT',
  'text:big'             => 'TEXT',
  'text:normal'          => 'TEXT',

$magic_map_or_list = [
  'varchar_ascii:normal' => 'VARCHAR',
  'varchar:normal' => 'VARCHAR',
  'char:normal' => 'CHAR',
  'text:tiny' => 'TEXT',
  'text:small' => 'TEXT',
  'text:medium' => 'TEXT',
  'text:big' => 'TEXT',
  'text:normal' => 'TEXT',

I think there are several points leading me toward the second option:
You can immediately see what they are assigned to.
The added burden of handling updates when something changes.
The longer the longest assignment is the further your eyes have to travel whitespace to find the assignment.
If the longest pushes it passed 80 characters, all would be beyond 80 characters.
Diffs for fixes space columns can be messy and hard to read, changing one requires all lines to update rather than just showing the addition or deletion.

As I read your comment I think "Yes, the second example is much harder to read and gets worse as it gets longer because it becomes harder and harder to parse the groupings of the assignments at a glance." but clearly that's not what you means so I don't think we agree.

But lets take your points and address them.

1. "The added burden of handling updates when something changes."
   This has been pointed out repeatedly and refuted as not be a big burden, and these lists seldom change and the change isn't hard. As an example, the sqlite code actually needs to be updated, it took me about a minute to make the patch. Half of that was remembering if it was ctrl-shift for multi-line select in jetbrains because I switch editors a lot. Most of the time you're going to be adding the smaller value and its trivial.
2. "The longer the longest assignment is the further your eyes have to travel whitespace to find the assignment."
   The longest gap in the updated sqlite it 13 characters. Previously it was 8. But that misses the point. The formatting means you don't have to scan back and forth, it lays it out in a big table so you can skim it and understand it at a glance.
3. "If the longest pushes it passed 80 characters, all would be beyond 80 characters."
   The sqlite example maxes at 43 characters long.
   The color example sited repeatedly is now quite a bit longer with language features, a long name, and a big array protected const ROTATE_TRANSPARENT = [255, 255, 255, 127];. That reaches a max of 61. At least the examples in core this doesn't seem likely to be a problem unless its some _deeply_ nested code and we should probably refactor that anyways.
4. "Diffs for fixes space columns can be messy and hard to read, changing one requires all lines to update rather than just showing the addition or deletion."
   a bunch of comments about whitespace. hide whitespace is still your friend.
   Reading this again I realize you mostly meant reading the surrounding code. Its a pet peeve of mine that people only read the change and not the surrounding code or the context of the code. When I assume something is simple and fall into that trap in code reviews it always introduces a bug. Every time. But in modern software development that seems to be a mostly me problem so not sure how to address that point.

The formatting means you don't have to scan back and forth, it lays it out in a big table so you can skim it and understand it at a glance.

I disagree with this, I find scanning the second example in #26 much easier than the first. The keys and values are closer together so much less eye movement.

It looks like we have found an issue that people are very much set in their ways about.
This is however something that we really need to find a rule for, because having both styles mixed in a codebase is not great in my opinion.

Is there a way we can get a poll out to the wider community to vote on this issue, we normally try to build consensus around coding standards issues, but I think this one might be difficult to find consensus on.

It looks like we have found an issue that people are very much set in their ways about. This is however something that we really need to find a rule for, because having both styles mixed in a codebase is not great in my opinion.

Huh? *No one* in the "allow whitespacing" camp is saying all grouped variables MUST be separated by columnar whitespace.

Huh? *No one* in the "allow whitespacing" camp is saying all grouped variables MUST be separated by columnar whitespace.

No that is true, but there are people (like me), that cannot stand the "allowing whitespacing" style, and there are people that find the "only 1 whitespace" allowed style to be a lot less readable. Maybe I am misunderstanding, English is not my first language.

As @borisson_ points out what we have now is a mixture of the two styles and that, in itself, can be confusing. For some, when reading a file with both styles it raises questions, such why multiple spaces here but not there. And worse, which style do I use when editing the file.

I was just scanning core and the block of code in \Drupal\taxonomy\TermViewsData::getViewsData is a mixture of both styles. For me, it is an example of where allowing both styles reduces readability and that this proposal is a benefit.

@quietone: Your example would NOT be forbidden with the proposed revision, as array key association operators are not specifically mentioned (that is: "=>" is not part of "All binary operators (operators that come between two values), such as +, -, =, !=, ==, >, etc. need to have exactly one space before and after the operator, for readability. Alignment with extra whitespace is not allowed.".

EDIT: I suppose this would be covered with "etc."

I was just scanning core and the block of code in \Drupal\taxonomy\TermViewsData::getViewsData is a mixture of both styles.

I would agree that that needs to be improved to use only one style EITHER single-spaced or whitespace-aligned. Does "mixing of both styles" mean "in a single code block" (like your example array)? If it does, I would support a rule that says "use either single-spaced or whitespace-aligned within a single code block; don't mix them." This would still allow both options overall, but with consistency for a single instance.

Personally I never want to see whitespace aligned.

It is different in an actual table cause there are lines for cell borders which is not the case here.

I really struggle with whitepace aligned assignments.

Added a Change Record to describe what this issue is trying to do. However we still currently have this at +7, -5 (if I counted correctly).

I feel very strongly that we need to find a way to void cases like the one mentioned in #32, and I think that is easy to find consensus around. I however don't think it's easy to write into a rule, both in text, as well as one that is automatable.

As I mentioned a year ago, I'd love to get more feedback on this issue.

I'm on a + for this, so make it +8, -5.

I think that having this consistant would be nicer than some more abritary rules for styling.

I am setting to Needs Work for an MR.