Add Generic.Array.ArrayCommaSpacingSniff

[VincentLanglet]

This sniff checks:

• There is no space before comma
• There is one space or newline after comma

Todo List for other sniff:

• Last element of singleline array has no comma
• Last element of multiline array has comma

[jrfnl]

Some review notes:

• The fixes will cause parse errors on PHP < 7.3 for heredoc/nowdoc.
• Short lists will also be touched by this sniff.

Personally, I think the "comma after last" check should be a separate sniff from the "spacing around comma's" check, which could actually be turned into a more generic spacing around comma's check independently of arrays.

[VincentLanglet]

• The fixes will cause parse errors on PHP < 7.3 for heredoc/nowdoc.

I don't know this but found this example on internet.

$values = [<<<END
a
b
c
END
, 'd e f'];

Did you talk about this ?

Short lists will also be touched by this sniff.

Is it wrong ?

Personally, I think the "comma after last" check should be a separate sniff from the "spacing around comma's" check, which could actually be turned into a more generic spacing around comma's check independently of arrays.

There is already Generic.Functions.FunctionCallArgumentSpacing and Squiz.Functions.FunctionDeclarationArgumentSpacing (used in PSR2 and PSR12) checking for space around comma. We will get multiples messages for the same error.

But I can split this into CommaAfterLast and ArrayCommaSpacing if needed.

[glen-84]

I think trailing commas in multi-line arrays should be an option, because not everyone uses them.

[VincentLanglet]

I think trailing commas in multi-line arrays should be an option, because not everyone uses them.

@glen-84 I'm sorry but I don’t understand. That’s the principe of a Sniff. You use it only if you want the check to be done. Did you mean the spacing check and the comma check should be split in two different sniffs ?

[glen-84]

That’s the principe of a Sniff. You use it only if you want the check to be done.

If that were true, then sniffs like Generic.WhiteSpace.ScopeIndent would have no options. Not everyone wants to use 4 spaces, just like not everyone wants to use trailing commas.

Some developers may want to enforce the opposite (no trailing commas), so an option like $trailingCommas (which could default to true), may be useful.

Anyway, you don't have to add it, it's just a suggestion.

[VincentLanglet]

Ok I understood what you mean. I added the option @glen-84.

I also added an option for hereDoc/nowDoc in order to avoid parse error with php < 7.3

[gsherwood]

I prefer different sniffs to do things like enforce a trailing comma or enforce no trailing comma, rather than have a property for the sniff. It makes it easier for people to understand when the sniff has a singular purpose, so new sniffs should try and do this.

I also agree that this specific rule (comma or not) should live alone inside a sniff and not be mixed up with spacing requirements.

Having said that, I think the array comma sniffs do actually need a property so that a developer can decide if they want commas required/banned after both single and multi-line arrays.

So I'd see 2 sniffs here: Generic.Arrays.EndComma and Generic.Arrays.DisallowEndComma

Both might have a property called $type (or something like that) that can be set to 'both', 'single', or 'multi'. Maybe a boolean would be better - I'm open to suggestions.

To use the sniffs to enforce no comma at the end of single-line array declarations and enforce a comma for multi-line definitions, you'd so something like this:

<rule ref="Generic.Arrays.EndComma">
    <properties>
        <property name="type" value="multi"/>
    </properties>
</rule>
<rule ref="Generic.Arrays.DisallowEndComma">
    <properties>
        <property name="type" value="single"/>
    </properties>
</rule>

Thoughts?

[VincentLanglet]

I was ok with two sniffs : one with spacing and one with trailing comma.

I understand your point of view, about creating two sniff but I'm disturbed with the following fact.
For what I know, there is a debate between require and disallow trailing comma for multiple array, but I never met someone who wanted trailing comma for single-line array.
Plus, I do like options when they have a default value.

If I create a Generic.Arrays.DisallowTrailingComma, I feel like everybody will use the
<property name="type" value="single"/> option, so this should be a default value. But then would it be clear for the user that using this sniff will automatically do the check for single line, but he need to use an option for the multiple lines array ?

And if I create a Generic.Arrays.RequireTrailingComma, I feel like people will only use this for multi line array. So should the <property name="type" value="single"/> be the default value ?

That's why maybe the best split of this sniff is :

• Generic.Arrays.SpacingCommaSniff
• Generic.Arrays.SingleLineTrailingCommaSniff, with an option to require/forbid (a boolean ?), with the default value set to forbid.
• Generic.Arrays.MultiLineTrailingCommaSniff, with an option to require/forbid (a boolean ?), wit the default value set to require (I think this is the most used standard).

What do you think about this split @gsherwood ?

[gsherwood]

That's why maybe the best split of this sniff is :

• Generic.Arrays.SpacingCommaSniff
• Generic.Arrays.SingleLineTrailingCommaSniff, with an option to require/forbid (a boolean ?), with the default value set to forbid.
• Generic.Arrays.MultiLineTrailingCommaSniff, with an option to require/forbid (a boolean ?), wit the default value set to require (I think this is the most used standard).

What do you think about this split @gsherwood ?

I'd be happy with that as well.

Assuming the property is called allow and the default is true, then you could implement it like this:

<rule ref="Generic.Arrays.MultiLineTrailingCommaSniff" />
<rule ref="Generic.Arrays.SingleLineTrailingCommaSniff">
    <properties>
        <property name="allow" value="false"/>
    </properties>
</rule>

[VincentLanglet]

I'd be happy with that as well.

Assuming the property is called allow and the default is true, then you could implement it like this:

<rule ref="Generic.Arrays.MultiLineTrailingCommaSniff" />
<rule ref="Generic.Arrays.SingleLineTrailingCommaSniff">
    <properties>
        <property name="allow" value="false"/>
    </properties>
</rule>

Ok, I'll work on it.

What would be the 'good' default value according to you ?

Based on usage, I think it would be :

• singleLine => allow = false
• multiLine => allow = true

But maybe consistency is preferred and then it's better to be

• singleLine => allow = false
• multiLine => allow = false

[VincentLanglet]

By the way, I think i didn't fix all the issue with HereDoc/NowDoc.

$values = [
<<<END
a
b
c
END
,
'd e f'
];

Is actually untouched.

But

$values = [
'def',
<<<END
a
b
c
END
];

Is fixed to

$values = [
'def',
<<<END
a
b
c
END,
];

I need to work on this too.

[gsherwood]

What would be the 'good' default value according to you ?

As per my example, I thought the default would be true for both.

[VincentLanglet]

What would be the 'good' default value according to you ?

As per my example, I thought the default would be true for both.

Isn't it weird having the default value to true SingleLineTrailingCommaSniff since everybody forbid trailing comma in single line array AFAIK ?
That's why I hesitate between false/true and false/false, but wasn't considering true/true.

But you have the final word.

[gsherwood]

Isn't it weird having the default value to true SingleLineTrailingCommaSniff since everybody forbid trailing comma in single line array AFAIK ?

Then you'd write a sniff called DisallowSingleLineTrailingCommaSniff and have no option, making the assumption that nobody will ever want this. I'd be happy with that option as well.

[VincentLanglet]

Ok ! I'll go with this.

[glen-84]

@VincentLanglet,

I apologize for my incorrect suggestion, I had no idea that Greg would want separate sniffs for this.

I had assumed that it may just look something like this:

<rule ref="Generic.Arrays.TrailingCommaSniff">
    <properties>
        <property name="singleLine" value="false" /> <!-- default -->
        <property name="multiLine" value="true" />   <!-- default (possibly false) -->
    </properties>
</rule>

It seems a bit strange to me to have separate sniffs for each variation (think SpaceAfterCast + NoSpaceAfterCast [doesn't exist], etc.), but I guess it doesn't really matter.

[VincentLanglet]

No problem, @glen-84.

@gsherwood I'll start with the ArrayCommaSpacingSniff.
I will make one PR per Sniff.

[VincentLanglet]

Hi @gsherwood ! Are you interested by this PR ? :)