Page MenuHomePhabricator

Add sniff to require trailing commas in multiline arrays
Closed, DeclinedPublic

Description

Adding a comma after the last array element when the closing bracket is on a separate line is standard best practice (makes merge conflicts and mistakes a little less frequent). We should have a sniff enforcing / fixing it.

Event Timeline

This behavior by default is ignored by PHP itself (I think it's a Hack syntax?) but I've read about it and there is one big advantage of allowing a trailing comma in a multiline array;

  • It speeds up or enables duplicating or adding more lines in the array easily. So, one doesn't have to care about comma for lines being added (after the last line previously) as once the last line in the array is copied, we focus on pasting (in the next lines) and don't worry on breaking the code (as there is already a comma after each addition), hence reduces the possibility of introducing errors by omitting a comma. For example, consider the array;
$array = [
    'a',
    'a',
    'a',
];

If we want more values of 'a' in the array, we just copy the last line and keep pasting and hitting return.

But for a case like;

$array = [
    'a',
    'a',
    'a'
];

We need to be very careful while adding new entries and not to forget adding a comma and this can actually break code :)

In T222042#5142599, @D3r1ck01 wrote:

We need to be very careful while adding new entries and not to forget adding a comma and this can actually break code :)

That's what linters are for ;)

This is one of these cases where I wonder if the effort is worth it. How often do we run into a situation where we think "dang, I wish a sniff would have reported this before". I can't recall having this problem. In cases where I have to add an array element, but there is no trailing comma, I can clearly see this in my IDE, because the code is literally broken otherwise and the IDE immediately highlights this.

I think I run into a merge-conflict because of a missing comma a single time in like five years.

Let me put it this way: This sniff absolutely must be auto-fixable, in 100% of the cases it reports.

That said, I prefer having these extra commas, and always typically add them to code I write:

$array = [
    'a' => 1,
    'b' => 2,
];

But not when the array is formatted like this:

$array = [ 'a' => 1, 'b' => 2 ];

But not when the array is formatted like this:

$array = [ 'a' => 1, 'b' => 2 ];

You're absolutely right per above, that is not a multiline array, it's uniline. :)

I just found a relevant use-case where I intentionally do not want an array to have a dangling comma, not even when it spans multiple lines:

/**
 * @return Revision[] An array with exactly two revisions, the previous and the current one.
 */
public function getRevisions() {
    return [
        $previousRevision,
        $currentRevision
    ];
}

Or:

public function provideTestCases() {
    return [
        'first test case' => [
            'first argument',
            'second argument'
        ],
        'second test case' => [
            'first argument',
            'second argument'
        ],
    ];
}

/**
 * @dataProvider provideTestCases
 */
public function test( $firstArgument, $secondArgument ) {
}

In both cases the arrays must contain exactly two elements. Not less. Not more. I do not want to make it "easier" for people to add a third array element. Not having the dangling comma marks the end of the array as not being expandable.

I would say about 5 to 10% of the code I write is like that. The problem is: I do not want to mark 5 to 10% of my code with // phpcs:… exceptions just to be allowed to keep the benefit of not having dangling commas in such situations. I will probably disable the sniff entirely. Or in other words: I feel this is an argument to not turn this into a strict rule.

Hm, I think this might be down to personal preference… I know that when I write data providers for tests, I always use a trailing comma in the arguments arrays. Adding another parameter to a test function isn’t unheard of, and even if I think it will always only have the current parameters, I still prefer to have the trailing comma everywhere.

In the getRevisions() example, I’m less sure, but I would probably still include a trailing comma. I don’t think omitting trailing commas should be used to discourage adding a third array element, it’s not a strong enough signal for that anyways (without a // phpcs:… exception comment, nothing tells me if the omission is intentional or not). Instead, the fact that the array is not expandable should be clear from the context.

This is one of these cases where I wonder if the effort is worth it. How often do we run into a situation where we think "dang, I wish a sniff would have reported this before". I can't recall having this problem. In cases where I have to add an array element, but there is no trailing comma, I can clearly see this in my IDE, because the code is literally broken otherwise and the IDE immediately highlights this.

I think I run into a merge-conflict because of a missing comma a single time in like five years.

Let me put it this way: This sniff absolutely must be auto-fixable, in 100% of the cases it reports.

That said, I prefer having these extra commas, and always typically add them to code I write:

$array = [
    'a' => 1,
    'b' => 2,
];

But not when the array is formatted like this:

$array = [ 'a' => 1, 'b' => 2 ];

Can do - if a sniff does indeed auto-fix 100% of the cases it reports, would it be accepted / is this proposal approved?

No, as argued a little lower. There are reasons to not have a dangling comma in multi-line arrays. It should be a human that makes the decisions about this.

Oh well, I wrote the sniff, but I guess its not needed - declining per discussion above