Forbid using more than one space around operators

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."
   This is a general problem for all white-space heavy changes. As the last couple comments pointed out these are not commonly changing so this is an edge case change but in the case you do find yourself having to make or review one of these changes, all git platforms have a wonderful solution that highlights exactly the changes you're looking for. Its going to save you a ton the next time you have to wrap some code in an if.
   • In Gitlab, on the top of changes tab in the preferences on the right you'll see hide whitespace changes. On commits its a "hide whitespaces changes" button.
   • Github has is too its just in the gear drop down.
   • Git locally you can provide -w to the diff and show commands.

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 spacing is not 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.". The proposed revisions are specifically about operators, not arrays.

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.