Squiz.Arrays.ArrayDeclaration sniff not very configurable

[JDGrimes]

The Squiz.Arrays.ArrayDeclaration sniff is somewhat generic, but attempting to configure it to disable some errors by placing this in a ruleset will inadvertently disable other errors too:

    <rule ref="Squiz.Arrays.ArrayDeclaration">
        <exclude name="Squiz.Arrays.ArrayDeclaration.ValueNotAligned" />
        <exclude name="Squiz.Arrays.ArrayDeclaration.KeyNotAligned" />
        <exclude name="Squiz.Arrays.ArrayDeclaration.DoubleArrowNotAligned" />
        <exclude name="Squiz.Arrays.ArrayDeclaration.ValueNotAligned" />
        <exclude name="Squiz.Arrays.ArrayDeclaration.CloseBraceNotAligned" />
        <exclude name="Squiz.Arrays.ArrayDeclaration.ValueNoNewline" />
        <exclude name="Squiz.Arrays.ArrayDeclaration.MultiLineNotAllowed" />
        <exclude name="Squiz.Arrays.ArrayDeclaration.SingleLineNotAllowed" />
    </rule>

Basically, I'm not concerned about the alignment rules, but I'd like to still get the other errors, including the NoComma error when there is a missing comma, as in the following code:

array(
    'foo' => 1,
    'bar' => 2 // <-- missing comma, should give error.
);

However, due to logic of the processMultiLineArray() method, this error won't be given if alignment errors are disabled. Actually, it wouldn't be given even if the alignment errors were enabled, because only one error is given per-index within the loop. After a violation is found, continue is called. Maybe this is intended behavior, but it makes the sniff less configurable. Could this maybe be made more generic? Maybe custom indentation schemes could be defined?

[gsherwood]

It's probably best not to use that sniff unless you want everything it provides. It doesn't split all those checks into various sniffs because the lead-up work to parse the array would take too long over a large code base when run multiple times for each array.

It also stop processing at various places to ensure it doesn't generate incorrect errors.

So yes, it is a complex and non-configurable sniff because it exactly implements a single method of defining arrays for the Squiz standard. I'll leave this open as a feature request, but I don't have any plans on making it configurable. I think it would be better to write a new Generic sniffs (or sniffs) to implement checks that are more commonly used.

[JDGrimes]

OK. I started thinking that maybe a flag could be introduced that would just turn off the indentation part. But I'd have to dig into the logic a little more to see if it would be possible to that cleanly without still having the sniff do lot's of extra work.

[frvge]

+1 for more configuration possibilities (I ended up removing the sniff because of the alignment). Maybe it's possible to cache things somewhere based on the start of the token?

[gsherwood]

Reposting a comment from #815:

There is no point making modifications to this sniff. The only reason people ask for it is because it is the only sniff that actually checks array formats, but the formatting is bespoke and annoying.

Setting the indent to 4 spaces wont help you because it would want it indented to exactly 4 spaces after the open array token. What you (and the majority of developers, including me) probably want is to have content indented to the next tab stop (based on your indent rules) no matter how many spaces that actually is. This lets you easily indent arrays by just tabbing as normal.

What I want to do is write a set of new array sniffs in the Generic standard to enforce a few key concepts. You can then use as many or as few as you want in a custom standard. The ones I am planning are:

• comma after all array values
• indent values to next tab stop
• align => in key/value pairs

Then I'll probably change the existing Squiz sniff to remove a lot of checks, or remove the sniff entirely if the job can be done with other sniffs.

[beporter]

@gsherwood That's great to hear. A sniff that can enforce trailing commas (on multi-line arrays only) has been on my todo list for a long time. Is there anything we can do to help or make the job easier for you?

[aik099]

A sniff that can enforce trailing commas (on multi-line arrays only) has been on my todo list for a long time.

I've created such sniff myself for my standard. If you want you can use it: https://github.com/aik099/CodingStandard/blob/master/CodingStandard/Sniffs/Array/ArraySniff.php

[sebastienbarre]

Thanks @gsherwood; after spending some time looking at that Sniff to find why some rules where not triggered (early return were to blame), I understand there is probably no point sending a PR to add a configuration option. I was wondering if these new array sniffs were somewhere in a branch maybe? Thank you.

What I want to do is write a set of new array sniffs in the Generic standard to enforce a few key
concepts. You can then use as many or as few as you want in a custom standard. The ones I am
planning are:
comma after all array values
indent values to next tab stop
align => in key/value pairs

[gsherwood]

I was wondering if these new array sniffs were somewhere in a branch maybe?

I haven't started them and I'm not sure when I'll get the time. But it's on my todo list.

[beryllium]

It would be really nice if double arrow alignment could be implemented as a separate sniff for 3.0, but I suppose it might be too much work/too late for that.

[jrfnl]

FYI: with the WordPress Coding Standards we've been running into more and more edge cases related to this sniff, so I've been splitting bits and pieces out from this sniff each time we run into another issue.

I expect nearly everything to be split off into independent sniffs before the end of the year. Once the split off sniffs have been tried & tested & found stable, I intend to send in pull requests for them here which would effectively solve this issue.

[beryllium]

Thanks for the update :) That's great news.

[gsherwood]

This is something I am considering for version 4 and not for version 3. I want to remove this sniff and replace it with a number of others - not necessarily one sniff per error message now that version 3 can include single messages in rulesets.

Whatever ends up happening, anyone using this sniff would need to change their ruleset to get the same functionality, so I'll provide that in an upgrade guide. I'd like to make version 4 the version where a lot of sniff codes change so it is a one-off process. This may mean moving more sniffs into the Generic standard, or breaking a few things out. But I'm not going to start on this for a while yet.

[glen-84]

comma after all array values

Could the sniff be configurable to enforce the opposite (no comma after the last element in a multi-line array)?

align => in key/value pairs

It would be great if this could make alignment optional, for example:

$x = [
    1 => 1
    999 => 999
]

... is okay because there is a single space before each =>.

$x = [
    1   => 1
    999 => 999
]

... is okay because the => are all aligned.

It should prevent mixing those styles as well. I guess starting from the top, as soon as an element uses more than one space before the =>, it goes into "alignment mode", and all => must then be aligned.

Also, it would be nice if it could enforce that the longest key had only a single space after it. i.e., this would be invalid:

$x = [
    1       => 1
    999     => 999
]

... because key 999 (the longest) should be followed by a single space.

[gsherwood]

@glen-84

Could the sniff be configurable to enforce the opposite (no comma after the last element in a multi-line array)?

The existing sniff wont, but I still need to write a new sniff that enforces a comma after the last item and it's a good idea to also allow it to enforce no comma as well.

It would be great if this could make alignment optional, for example:

I see what you're getting at, but I wasn't intending to do anything like this in a new double array alignment sniff. Standards are better when the developer doesn't have to make a decision and the sniff doesn't have to infer anything from how the code is written.

One suggestion would be to figure out why you wouldn't want the arrows aligned and then code something like that into a sniff. For example, the sniff that aligns equals signs under each other when you are assigning multiple variables in a block knows that sometimes variable names are too long and the number of spaces you need to align an equals sign gets ridiculous. So it has a max alignment setting that says "if I need to use more than N spaces to align the equals sign, use a single space instead". It's not a decision the developer makes - they can't decide to align it anyway.

Something to think about maybe.

Also, it would be nice if it could enforce that the longest key had only a single space after it. i.e., this would be invalid:

The existing sniff already does this. The code sample you posted generates these errors:

  3 | ERROR | [x] Array double arrow not aligned correctly; expected 3 space(s) but found 7 (Squiz.Arrays.ArrayDeclaration.DoubleArrowNotAligned)
  4 | ERROR | [x] Array double arrow not aligned correctly; expected 1 space(s) but found 5 (Squiz.Arrays.ArrayDeclaration.DoubleArrowNotAligned)

[glen-84]

One suggestion would be to figure out why you wouldn't want the arrows aligned ...

I just don't think that it always make sense, especially in larger array declarations. It can become a bit of a pain to format everything correctly (assuming no cbf/prettier/etc.).

I suppose max spaces/array length settings could help, but optional alignment seems useful as well.

[RobQuistNL]

How is this still open 😅

[ncou]

hi,
nice piece of code. After a few tweak, it seems to me the check to enforce a space after the comma is not working. I should have an error with the "NoSpaceAfterComma" check but phpcs doesn't detect the missing space :(

[gsherwood]

From #2687

Help, I need check a whitespace after comma in single line array/list.

Code:

[$a,$b,$c] = ['a','b','c'];

// convert to

[$a, $b, $c] = ['a', 'b', 'c'];
Version: 3.5.1

I saw the Squiz.Arrays.ArrayDeclaration source code, and config Squiz.Arrays.ArrayDeclaration.SpaceAfterComma rule, but it's does not work.