[Policy, no patch] Within the Function Calls section, delete explicit mention of padding spacing in a block of related assignments

That note under Function Calls contradicts the Operators standards on the same page:
https://www.drupal.org/coding-standards#operators
So my opinion is that this note about it being allowable to line things up should just be removed, so that the Function Calls standard does not contradict the Operators standard. As it is now, they contradict each other.

Also as a note, I do not think that we have code in Drupal Core (at least, no recent code) that tries to line itself up like that... I don't think anyone knew we had that as a possibility. We just don't do it like that, and I think if you submitted a patch like that to Drupal Core it would not get committed.

+1 for the change. Did not even know that was in the documents. And you are right, if a patch would have been submitted like that it would have been rejected for sure.

I noticed that section before, but am also in favor of removing it. It's a bit mysterious why it would even be there, variable assignment doesn't really have anything to do with function calls.

I'd maintain, though, that it doesn't really contradict the operators standards: there, it says that there should be a space on both sides of binary operators – but it doesn't say anything about putting more than one there. So maybe we should, additionally, specify there (under "Operators") that it should be a single space on both sides:

All binary operators (operators that come between two values), such as +, -, =, !=, ==, >, etc. MUST have exactly one space before and after the operator, for readability. For example, an assignment should be formatted as $foo = $bar; rather than $foo=$bar;. Unary operators (operators that operate on only one value), such as ++, should not have a space between the operator and the variable or number they are operating on.

Or we could add a small second paragraph explicitly forbidding using additional whitespace to line up assignments.

Awww.... Please no. This can really improve readability in some cases. The function call seems like the section that is unneeded and should be deleted. There is no reason to special case them from any other assignments.

Can we have a precise proposal in the issue summary at which place in the coding standards which sentence/block would change and into what?

-1.

Diffs and merges are one-time events for a small subset of (then active) developers.

Reading the final committed code is an always-event for (all interested) developers.

If whitespace makes the always-event more readable, then we should not cater to the one-time event.

My mistake about the operators, I misunderstood the argument. I agree it should not change but only because "should have a space before and after the operator, for readability" does not preclude additional spaces for readability. Pretty sure that is by design not a contradiction.

I understand the merge argument, I just don't think its a big enough chance for collision to preclude using it for readability when it is warranted. If the issue was changed to propose clarifying/discouraging its use and identifying the risk I would be more on board.

I disagree with @Morbus Iff and @neclimdul, the merge argument is a very strong one. It's hard enough to get people engaged in the issue queues and if we can avoid having to do rerolls that's a good thing. I personally don't even see the big benefit in readability.

The TWG discussed this issue and we think we need more support from active maintainers. Can we get more sponsorship from module maintainers and maybe a core committer?

As facets maintainer: +1

I maintain several modules and have already given my +1

@Perignon - I meant no disrespect - we weren't saying that there was *no* sponsorship from module maintainers - just looking for a bit more. :-)

-1 from me -- I agree with the reasoning in #12.

+0.5 from me, I hate the case when I have to add a new variable that is longer. I'd rather that either way be acceptable, because in the list of things I care about with code standards, this is generally at the bottom.

I can see the argument for allowing it, although I think in the last 6 years I've done that sort of block formatting exactly never. I guess I'm also +0.5 for removal of that style. There's definitely bigger fish to fry, though, as Dave said.

@davereid the multicursor feature available in most IDE's and advanced editors makes that a lot less painful. You can just drop a cursor on any number of lines and change in indenting in a flash. No pain. :) https://www.jetbrains.com/help/idea/2016.2/multicursor.html

I would still like more clarity from the summary. The first part of the summary suggests we are precluding its use while the proposed change only removes the words allowing it. In my interpretation not being excluded means its still ok so we're not really changing anything just arguing over shortening a docs page. But if I'm back here in 3 months because someone is blocking my patch or opening issues against one of my modules I'm going to be really annoyed. So I'm +1 for shortening our docs and strongly -1 for blocking developers for using indenting but still don't know where I stand on the issue.

Since I didn't see it mentioned anywhere here's some quick research I did into other communities.

• Ruby: Found no specific allowances but default rubocop will fail if indenting is not aligned and pass if indenting is aligned. So defacto it seems allowed.
• Python: Not "excluded" but to be avoided as "Pet Peeve" https://www.python.org/dev/peps/pep-0008/#pet-peeves
• Java: Not really familiar with options here but Google very specifically allows it but notes drawback https://google.github.io/styleguide/javaguide.html#s4.6.3-horizontal-ali... (I like how its both explicitly permitted but clearly documents drawbacks)
• PHP: PSR2 say "The use of spaces also makes it easy to insert fine-grained sub-indentation for inter-line alignment." but specifically avoids making and requirements or suggestions about its usage.

One of my concerns about

  // Rather than this:
  $short         = foo($bar);
  $long_variable = foo($baz);
  // So all the lines do not change if this is rewritten:
  $short                = foo($bar);
  $even_longer_variable = foo($biz);
  $long_variable        = foo($baz);

Is that when naming that $even_longer_variable you are tempted to choose a shorter name just to keep from having to the change the other lines. Or, you forgot to change all the other lines. I'm +1 on the change for those reasons.

Wow... I might be tempted to make an _even_shorter_ and make everything one word so it mostly lines up with the enforced non-indention. So maybe blocking indention is worse?

So, I'm guessing basing on other work going on that the use of phpcs/coder will become more and more prevalent. Catching mistakes in indentation like that can then be automated and caught up front. In which case my experience in Ruby suggests that you end up choose the places to use this indention carefully based on very specific need for grouping and clarity because it will be fiddly. Using it for specific clarity and grouping is exactly what we've been arguing for though so that works out pretty well.

So I found a great example that shows some interesting things.

class ToolkitGdTest extends KernelTestBase {
  // ...

  // 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);
  // Used as rotate background colors.
  protected $fuchsia            = array(255, 0, 255, 0);
  protected $rotateTransparent = array(255, 255, 255, 127);

  protected $width = 40;
  protected $height = 20;

http://cgit.drupalcode.org/drupal/tree/core/tests/Drupal/KernelTests/Cor...

In this code we see some clearly grouped colors. Then we see some additional variables and... oh woops someone broke the indenting and those last two aren't actually a new section. While this clearly supports the fact that there is an additional cost identified in doing this that's not really being debated. The fact is the first block is incredibly clear and easy to scan and the fact that the other properties are not indented hampers reading. Infact because the variables include some longer and shorter values this becomes useful.

Why I support removing the line though is because the indention this example has nothing to do with function calls. Neither do any usages in core from what I can tell. We should just continue allowing additional whitespace for clarity and remove this odd association with function calls.

Personally this gets +1 from me, the non-spaced version is by far the most commonly used.

I agree with #25 that this is unrelated to function calls, and should not be mentioned in that section. This is about variable assignments.

Current tally:

Against:

1. neclimdul #2687941-8: [Policy, no patch] Within the Function Calls section, delete explicit mention of padding spacing in a block of related assignments
2. Morbus Iff #2687941-10: [Policy, no patch] Within the Function Calls section, delete explicit mention of padding spacing in a block of related assignments
3. Joachim #2687941-20: [Policy, no patch] Within the Function Calls section, delete explicit mention of padding spacing in a block of related assignments

In favor:

1. Perignon #2687941-4: [Policy, no patch] Within the Function Calls section, delete explicit mention of padding spacing in a block of related assignments
2. drunken monkey #2687941-6: [Policy, no patch] Within the Function Calls section, delete explicit mention of padding spacing in a block of related assignments
3. FatherShawn #2687941-11: [Policy, no patch] Within the Function Calls section, delete explicit mention of padding spacing in a block of related assignments
4. borisson_ #2687941-17: [Policy, no patch] Within the Function Calls section, delete explicit mention of padding spacing in a block of related assignments
5. Dave Reid +0.5 #2687941-21: [Policy, no patch] Within the Function Calls section, delete explicit mention of padding spacing in a block of related assignments
6. Crell +0.5 #2687941-22: [Policy, no patch] Within the Function Calls section, delete explicit mention of padding spacing in a block of related assignments
7. alexpott #2687941-24: [Policy, no patch] Within the Function Calls section, delete explicit mention of padding spacing in a block of related assignments
8. pfrenssen #2687941-26: [Policy, no patch] Within the Function Calls section, delete explicit mention of padding spacing in a block of related assignments

We still have the question about "one space" vs "a space". The current standards mention both (emphasis mine):

From Operators:

All binary operators (operators that come between two values), such as +, -, =, !=, ==, >, etc. should have a space before and after the operator, for readability. For example, an assignment should be formatted as $foo = $bar; rather than $foo=$bar;.

From Function calls:

As displayed above, there should be one space on either side of an equals sign used to assign the return value of a function to a variable.

To me it sounds like the original intention was to have one single space before and after - this seems to make sense for the mathematical and comparison operators +, _, =, !=, ==, <, >= (which are not typically aligned), and by extension also to the assignment operator =. Also, because the section under Function Calls explicitly mentions "one space on either side of an equals sign" this is already the official standard for the assignment operator. We currently have an exception to this rule that allows for aligning the assignments, but this exception is being removed as of this issue.

So to clarify the ambiguity, we should probably update the Operators section to mention "one space" instead of "a space".

Perhaps there could be an exception for conveying code semantics. There are times when extra spaces reinforce the semantics of the code. For example:

$alpha = some_function(BLUE,  TRUE,  17,  $some_blue_option,  45);
$beta  = some_function(GREEN, FALSE, 100, $some_green_option, 4);
$gamma = some_function(RED,   TRUE,  -1,  $some_red_option,   NULL);

This happens probably more frequently when defining nested arrays and in order to understand the meaning, it helps to align the sub-elements from one super-element to the next. An example might be defining the table for a finite state machine.

In these cases, it is worth the small risk of a merge conflict and the extra few seconds to re-indent a bunch of lines when modifying the code.

I use tab stops for this sort of indenting, which gives me some hope of not having to re-indent a bunch if the next edit is only one character longer.

-1. I MUCH prefer lining up the spacing. Makes debugging significantly easier.

+1. I am for deleting the section as I tend to let automatic code formaters do their thing.

I don't want my colleagues' code show up in my Git commit, just because I pressed Alt + Shift + F in NetBeans.

@Gogowitsch My thoughts too. A tool I use a lot (in PHPStorm though).

I use tab stops for this sort of indenting, which gives me some hope of not having to re-indent a bunch if the next edit is only one character longer.

Interesting question, is that even allowed per our coding standards?
Seems to me a bad idea, since using tab stops within lines of code gives completely different results for different tab size settings.
However, it seems this is indeed currently allowed per our standards. I've opened #2795237: Explicitly forbid tab stops within lines to discuss this.

@drunken monkey -- By "tab stop", I mean the editor inserting the correct number of spaces to arrive on the standard 2-spaces-per-tab-stop character column. I'm not talking about ASCII character code 9.

Re automatic code formatters. I bet the undo semantic indenting of conditionals, such as:

  if ($some_long_conditional_prerequisite &&
      ($special_exception_situation || $another_conditional) &&
      $last_tricky_thing_that_must_be_true) {

An expression such as this is much easier to understand because the formatting reflects the semantics.

We discussed this on the Sep. 6 coding standards committee call. Sorry it's taken me this long to post this comment from that.

As far as we could tell, there was pretty much consensus on updating the docs as proposed in the issue summary, so I just now did that: https://www.drupal.org/node/318/revisions/view/10116963/10117001. I'm retitling this issue to scope it to just that.

There remains an open question of how to interpret the Operators section:

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

On the one hand, #11 says:

It seems like stretching the language out of shape to make "a space" mean "one or more spaces"

On the other hand, #13 says:

"should have a space before and after the operator, for readability" does not preclude additional spaces for readability.

We think there's merit to both those interpretations. https://en.wikipedia.org/wiki/Article_(grammar)#Indefinite_article points out that a/an are always singular. But https://www.drupal.org/about says "If, instead, you're looking for a new employee or freelancer, try the best place for hiring great Drupal talent: Drupal Jobs.", and we're pretty sure that this is intended to apply to someone who is looking for more than one new employee or freelancer as well.

We discussed this, and at this time, the Coding Standards committee supports the interpretation that "should have a space" DOES NOT prohibit having more than one space. So if anyone on this issue wants to push for an additional change to the documentation to make such a prohibition explicit, please open a new issue for that.

Here is the followup that discusses whether it is fine to use one or more spaces, or one space only.