Problem with NormalizedArrays.Arrays.CommaAfterLast in some cases #283
Description
Bug Description
The NormalizedArrays.Arrays.CommaAfterLast does its job pretty well. But there is a case that is hitting us a lot (because of our own coding standard explicitly allowing it), and that I think that can be hitting others out there too.
Right now we support all these arrays as valid:
$a = [1, 2, 3, 4]; // Normal mono-line.
$a = [
1,
2,
3,
4,
]; // Normal multi-line
$a = [1, 2,
3, 4]; // Multi-multi line with same line closer (to call it some way).
$a = [
1, 2,
3, 4,
]; // Multi-multi line with closer apart (also to call it some way).
And the Sniff is working ok for all the cases but the 3rd one. And it seems logic not to require a comma there, because, if there are new items to be added to the array later, the line is going to change, yes or yes (the closer will need to be moved.
Given the following reproduction Scenario
Ensure that "Multi-multi line with same line closer " cases don't require a comma.
The issue happens when running this command:
vendor/bin/phpcs --standard=NormalizedArrays --sniffs=NormalizedArrays.Arrays.CommaAfterLast test.php... over a file containing this code:
<?php
$a = [1, 2,
3, 4];
$a = [1 => 1, 2 => 2,
3 => 3, 4 => 4];
$a = [1 => [1 => 1,
2 => 2,
3 => 3,
4 => 4,
], 5 => 5];
$a = [1 => [1 => 1,
2 => 2,
3 => 3,
4 => 4],
5 => 5];I'd expect the following behaviour
No errors are reported.
Instead this happened
Five errors are reported.
-------------------------------------------------------------------------------------------
FOUND 5 ERRORS AFFECTING 5 LINES
-------------------------------------------------------------------------------------------
4 | ERROR | [x] There should be a comma after the last array item in a multi-line array.
7 | ERROR | [x] There should be a comma after the last array item in a multi-line array.
13 | ERROR | [x] There should be a comma after the last array item in a multi-line array.
18 | ERROR | [x] There should be a comma after the last array item in a multi-line array.
19 | ERROR | [x] There should be a comma after the last array item in a multi-line array.
-------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 5 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-------------------------------------------------------------------------------------------
Environment
| Environment | Answer |
|---|---|
| PHP version | Any |
| PHP_CodeSniffer version | 3.7.2 |
| PHPCSExtra version | 1.1.1 |
| PHPCSUtils version | 1.0.8 |
| Install type | Normal composer, used by our standard |
Additional Context (optional)
This was originally reported for us @ moodlehq/moodle-cs#76
And this change seems to fix the problem:
diff --git a/NormalizedArrays/Sniffs/Arrays/CommaAfterLastSniff.php b/NormalizedArrays/Sniffs/Arrays/CommaAfterLastSniff.php
index d03d1ff..6472e5c 100644
--- a/NormalizedArrays/Sniffs/Arrays/CommaAfterLastSniff.php
+++ b/NormalizedArrays/Sniffs/Arrays/CommaAfterLastSniff.php
@@ -159,6 +159,12 @@ final class CommaAfterLastSniff implements Sniff
return;
}
+ // If the line of the last non-empty token is the same as the closer, we're not
+ // going to require a comma. TODO: Put this under some Sniff configuration setting.
+ if ($tokens[$lastNonEmpty]['line'] === $tokens[$closer]['line']) {
+ return;
+ }
+
$error = 'There should be a comma after the last array item in a %s array.';
$errorCode = 'Missing' . $errorCode;
$data = [$phrase];
Note the TODO that I've put there. IMO there are 4 alternatives:
- We make that the default behaviour. After all, it doesn't make sense to force commas when the closer is in the same line.
- We make it a new
$validValues, say:enforce_but_when_sameline_closer. - We make it a new, apart sniff setting:
enforce_skipped_when_sameline_closer. - The change is not acceptable (so we'll have to duplicate the sniff into our standard and apply the patch there.
If there is any insight/preference about any of the 1-3 alternatives above... I'm happy preparing a PR with tests and so on...
Ciao :-)
Tested Against develop branch?
- I have verified the issue still exists in the
developbranch of PHPCSExtra.