Skip to content

Commit 0e0e0fb

Browse files
committed
Squiz/FunctionDeclarationArgumentSpacing: improve SpaceBeforeComma fixer
The fixer for the `SpaceBeforeComma` error code would only handle one whitespace token at a time, which is inefficient and could also lead to more complex fixer conflicts. This commit changes the fixer to handle all whitespace tokens in one go. For the test code sample added in the previous commit, this means the difference between the fixer needing 4 loops versus 2 loops. Additionally, this commit updates the fixer to cleanly handle moving the comma in the case of a trailing comment between the end of the previous parameter and the comma. Tested via the `newlineBeforeCommaShouldBeFixedInOneGo()` test case and the new `newlineBeforeCommaFixerRespectsComments()` test case.
1 parent 93b7e2d commit 0e0e0fb

4 files changed

+51
-3
lines changed

Diff for: src/Standards/Squiz/Sniffs/Functions/FunctionDeclarationArgumentSpacingSniff.php

+29-3
Original file line numberDiff line numberDiff line change
@@ -330,9 +330,35 @@ public function processBracket($phpcsFile, $openBracket)
330330

331331
$fix = $phpcsFile->addFixableError($error, $commaToken, 'SpaceBeforeComma', $data);
332332
if ($fix === true) {
333-
$phpcsFile->fixer->replaceToken(($commaToken - 1), '');
334-
}
335-
}
333+
$startOfCurrentParam = $phpcsFile->findNext(Tokens::$emptyTokens, ($commaToken + 1), null, true);
334+
335+
$phpcsFile->fixer->beginChangeset();
336+
$phpcsFile->fixer->addContent($endOfPreviousParam, ',');
337+
$phpcsFile->fixer->replaceToken($commaToken, '');
338+
339+
if ($tokens[$commaToken]['line'] === $tokens[$startOfCurrentParam]['line']) {
340+
for ($i = ($commaToken + 1); $tokens[$i]['code'] === T_WHITESPACE; $i++) {
341+
$phpcsFile->fixer->replaceToken($i, '');
342+
}
343+
} else {
344+
for ($i = ($commaToken - 1);
345+
$tokens[$i]['code'] === T_WHITESPACE && $tokens[$endOfPreviousParam]['line'] !== $tokens[$i]['line'];
346+
$i--
347+
) {
348+
$phpcsFile->fixer->replaceToken($i, '');
349+
}
350+
351+
for ($i = ($commaToken + 1);
352+
$tokens[$i]['code'] === T_WHITESPACE && $tokens[$commaToken]['line'] === $tokens[$i]['line'];
353+
$i++
354+
) {
355+
$phpcsFile->fixer->replaceToken($i, '');
356+
}
357+
}
358+
359+
$phpcsFile->fixer->endChangeset();
360+
}//end if
361+
}//end if
336362

337363
// Don't check spacing after the comma if it is the last content on the line.
338364
$checkComma = true;

Diff for: src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc

+10
Original file line numberDiff line numberDiff line change
@@ -185,3 +185,13 @@ function newlineBeforeCommaShouldBeFixedInOneGo(
185185
,
186186
$paramC
187187
) {}
188+
189+
function newlineBeforeCommaFixerRespectsComments(
190+
$paramA // comment
191+
,
192+
$paramB=10 /* comment */
193+
,
194+
$paramC=20 # comment
195+
, $paramC=30
196+
, string $paramC='foo'
197+
) {}

Diff for: src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.inc.fixed

+8
Original file line numberDiff line numberDiff line change
@@ -163,3 +163,11 @@ function newlineBeforeCommaShouldBeFixedInOneGo(
163163
$paramB,
164164
$paramC
165165
) {}
166+
167+
function newlineBeforeCommaFixerRespectsComments(
168+
$paramA, // comment
169+
$paramB=10, /* comment */
170+
$paramC=20, # comment
171+
$paramC=30,
172+
string $paramC='foo'
173+
) {}

Diff for: src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.php

+4
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,10 @@ public function getErrorList()
8080
174 => 2,
8181
182 => 1,
8282
185 => 1,
83+
191 => 1,
84+
193 => 1,
85+
195 => 1,
86+
196 => 1,
8387
];
8488

8589
}//end getErrorList()

0 commit comments

Comments
 (0)