Enforcing equal white space between associative arrays key => value is not readable in many cases

[azaozz]

Example:

'long-array-key-that-would-make-for-a-long-line-and-there-is-no-way-to-wrap-it-up' => 'value',
'key'                                                                              => 'long-array-key-value-that-is-not-easy-to-read-as-it-makes-the-line-too-long',

That spacing is contrary to the WP coding standards which say:

Exception: if you have a block of code that would be more readable if things are aligned, use spaces

Always enforcing aligned => for associative arrays breaks the above, as it makes them less readable.

Thinking that this rule should be removed (alignment left at the author discretion) or changed to warn about aligning only when it makes sense, i.e. there are 3-4 spaces difference.

[jrfnl]

@azaozz That sniff is very configurable, so you can make it work exactly the way you want already.

See:

• Array alignment: allow for new lines
• Array alignment: allow non-exact alignment
• Array alignment: maximum column
• Array alignment: dealing with multi-line items

[lkraav]

Thinking that this rule should be removed (alignment left at the author discretion) or changed to warn about aligning only when it makes sense, i.e. there are 3-4 spaces difference.

I like keeping the rule and adding a quantifier idea, because if teams have already done a bunch of work to standardize by large majority of sane-length keys, and expect automation to uphold it, it would be counter-effective to pull the rug later.

[azaozz]

@lkraav would it be possible to not throw a warning in "test" mode but still apply the changes when reformatting/fixing? That way we won't break current functionality, but can leave the decision "to align or not to align" to the author when only testing.

I'm also thinking there should be couple of levels of warnings. One that would trigger a "fail" when testing certain code and another that would print output to the terminal but not trigger a "fail". Then we can have some things as "recommendations", the same way they are recommendations in the WP standards.

@jrfnl uh, that also seems to "break" the WP standards:

To avoid excessively long lines, sometimes the array assignment operator will be on a new line

It makes for less readable arrays as completely breaks the "flow" of the =>. Example:

'long-array-key-that-would-make-for-a-long-line-and-there-is-no-way-to-wrap-it-up' =>
          'long-array-key-value-that-is-not-easy-to-read-as-it-makes-the-line-too-long',
'key' => 'value',

Again, this should probably be used at author discretion.

https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/wiki/Customizable-sniff-properties#array-alignment-allow-non-exact-alignment should probably be made conditional on the array being aligned, or perhaps should be off by default?

https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/wiki/Customizable-sniff-properties#array-alignment-maximum-column should be reduced further for core, perhaps to around 30 chars. Also, it should look at tabs in front of the array key and count them as 4 spaces (if it doesn't already :) ).

https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/wiki/Customizable-sniff-properties#array-alignment-dealing-with-multi-line-items would probably need to be extended to not align the => when the difference is over certain value, to avoid cases like the example in this issue. Thinking that the default should be a fairly low value for that difference, i.e. do not align if it would add more than few spaces. Then the resulting code will be pretty much like the example in the WP standards :)

In any case, seems this whole sniff should be turned off by default for code as the coding standards there clearly say that using spaces to align the = and => is an exception, not a rule :)

[jrfnl]

I'm also thinking there should be couple of levels of warnings. One that would trigger a "fail" when testing certain code and another that would print output to the terminal but not trigger a "fail". Then we can have some things as "recommendations", the same way they are recommendations in the WP standards.

That is not how PHPCS currently works. Please feel free to open a feature request in the PHPCS repo for this.

uh, that also seems to "break" the WP standards:

IIRC, it doesn't say anywhere in the standards that new lines around the double arrow aren't allowed.

array-alignment-allow-non-exact-alignment should probably be made conditional on the array being aligned, or perhaps should be off by default?

This was discussed when the sniff was originally created and rejected: https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/1122#issuecomment-327045377

array-alignment-maximum-column should be reduced further for core, perhaps to around 30 chars. Also, it should look at tabs in front of the array key and count them as 4 spaces (if it doesn't already :) ).

Of course tabs are translated to spaces already, that's a given.

The current default for Core was based on tests run against the WP core codebase, finding a balance between letting the line length not grow too much and still improving readability by having arrows aligned.

30 IMHO will be very low - think:

class ABC {
    function something( $a ) {
        switch ($a ) {
            case 'abc':
                $array = array(
                    'starting point is 20' => 'not aligned',
                );
                break;
        }
    }
}

array-alignment-dealing-with-multi-line-items would probably need to be extended to not align the => when the difference is over certain value, to avoid cases like the example in this issue.

I think you misunderstand that property. The maxColumn property is still respected, no matter what is set for this property.

The example you post above would never be aligned like that by this sniff, unless you changed the maxColumn value, so that is not an issue with the sniff itself.

[azaozz]

That is not how PHPCS currently works.

True, and that prevents it from making "suggestions" like in the WP standards.

IIRC, it doesn't say anywhere in the standards that new lines around the double arrow aren't allowed.

True again, but there are many things not defined there. For example: many nested function calls look pretty bad in some cases, and are very hard to read/understand especially for novice users (order of execution is lost). Also, the above rule breaks the underlying intention for code to be easy to read :)

This was discussed when the sniff was originally created and rejected:

Yeah, that rule should probably have a setting to enforce exactly one space before and after => and =. This is what the WP standards currently say, with an exception that there may be few more spaces before if it makes sense.

I think you misunderstand that property.

Uh, sorry, should have explained better, didn't mean an existing property.

I meant that the rules for alignment of => and = would have to be extended. Another rule that is probably needed there is to not change the code (and not throw warnings) if the number of white spaces that need to be inserted is too great. That often makes for pretty hard to read code. Example:

$array = array(
    'key'                                                                              => 1,
    'key2'                                                                             => 2,
    'key3'                                                                             => 3,
    'key4'                                                                             => 4,
    'long-array-key-that-would-make-for-a-long-line-and-there-is-no-way-to-wrap-it-up' => 5,
);

This gets a lot worse with multidimensional arrays:

$array = array(
    'key'                                                                              => array(
        'key'        => 'value',
        'key'        => 'long-array-value-that-would-make-for-a-long-line-and-there-is-no-way-to-wrap-it-up',
        'longer-key' => 'value',
    ),
    'key2'                                                                             => 2,
    'key3'                                                                             => array(
        'key'                                      => 'value',
        'key'                                      => 'long-array-value-that-would-make-for-a-long-line-and-there-is-no-way-to-wrap-it-up',
        'long-key-name-that-pushes-the-value-away' => 'value',
    ),
    'key4'                                                                             => 4,
    'long-array-key-that-would-make-for-a-long-line-and-there-is-no-way-to-wrap-it-up' => 5,
);

[jrfnl]

@azaozz Again, your examples would not be fixed that way by this sniff.

Please read the property descriptions carefully, cause all this can already be configured.

[azaozz]

@jrfnl hm, I may be missing something... Where can I set the maximum number of white spaces that will be inserted between key and => when aligning arrays? Example: an array formatted like this:

array(
    'key' => 'long-array-value-that-would-make-for-a-long-line',
    'long-key-name-that-pushes-the-value-away' => 'value',
)

should not be changed into this:

array(
    'key'                                      => 'long-array-value-that-would-make-for-a-long-line',
    'long-key-name-that-pushes-the-value-away' => 'value',
)

Also, my concerns here are mostly about throwing warnings when checking code. Currently it seems that I have to format the above examples in that way to avoid these warnings. That's not right :)

[jrfnl]

@azaozz

Where can I set the maximum number of white spaces that will be inserted between key and => when aligning arrays?

Sounds like you are looking for a maxPadding property. When this sniff was created, I made a decision not to implement that in favour of the maxColumn property - see https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/1122#issuecomment-328345556 for the more detailed reasoning.

Also, my concerns here are mostly about throwing warnings when checking code.

Well, there a reason that it's a warning and not an error. You can easily ignore warnings (or even turn the sniff off completely).

Currently it seems that I have to format the above examples in that way to avoid these warnings. That's not right :)

Well, what's the point of having a sniff if it doesn't do anything ? It can't very well say - "Well, in this case it looks reasonable enough, I will make an exception and not throw a warning this time".... That's not how it works.

[azaozz]

Well, what's the point of having a sniff if it doesn't do anything...

Well, it is a sniff that is enforcing an exception (and can produce unreadable, ugly code) :)

1. The WP coding standards clearly say there should be one space around = and =>. This part of the sniff is "enforceable", i.e it should throw an error if there is no space and possibly a warning when there is more than one space.

2. The WP coding standards also clearly say that the above rule can be broken by adding more white space before the = and => but only if that makes the code more readable.

The current sniff breaks # 2 as it makes it non-optional (contrary to the coding standards). Furthermore it sometime enforces code that is less readable than the original that was produced by following rule # 1 above).

In that terms everything except # 1 above should be "off" by default, and preferably re-examined to truly implement the WordPress Coding Standards, and not go against them.

[peterwilsoncc]

I've been considering this issue in the wider context of white space alignment for

• dockblocks
• leading up to =
• leading up to =>

One of the reasons WordPress uses tabs rather than spaces for intending code is that it's more accessible. A user can configure how wide each tab is displayed.

For users of braille output devices, this allows them to set the tab width to one character as they are often narrower than the typical screen. According to Vision Australia braille outputs are most commonly "12, 20, 32 and 40 or 80 braille cells" wide.

Limiting my search to the src directory & excluding vendor code:

• within docblocks, the widest indentation I found was 78 characters.
• In the lead up to an = sign the widest number of spaces I could find was 38 characters.

These values represent a significant proportion of a braille output according to the document linked above.

If the some of the WP Coding Standards are to be reconsidered, it would be good to discuss this as part of the process. I'm happy to split docblocks and code alignment in to separate issues if it would help.