Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Squiz.WhiteSpace.LanguageConstructSpacing does not properly check for tabs and newlines #1573

Merged
merged 4 commits into from
Nov 27, 2017
Merged

Squiz.WhiteSpace.LanguageConstructSpacing does not properly check for tabs and newlines #1573

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0e9b5bc
Hotfix for cases when next character is not a single space
michalbundyra Jul 24, 2017
14b9857
Added check if the next token is set
michalbundyra Jul 24, 2017
ce27026
Removed unused variable
michalbundyra Jul 24, 2017
5324d96
Added one more test case - space and new line
michalbundyra Jul 26, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions src/Standards/Squiz/Sniffs/WhiteSpace/LanguageConstructSpacingSniff.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

use PHP_CodeSniffer\Sniffs\Sniff;
use PHP_CodeSniffer\Files\File;
use PHP_CodeSniffer\Util;

class LanguageConstructSpacingSniff implements Sniff
{
Expand Down Expand Up @@ -50,17 +51,21 @@ public function process(File $phpcsFile, $stackPtr)
{
$tokens = $phpcsFile->getTokens();

if (isset($tokens[($stackPtr + 1)]) === false) {
// Skip if there is no next token.
return;
}

if ($tokens[($stackPtr + 1)]['code'] === T_SEMICOLON) {
// No content for this language construct.
return;
}

if ($tokens[($stackPtr + 1)]['code'] === T_WHITESPACE) {
$content = $tokens[($stackPtr + 1)]['content'];
$contentLength = strlen($content);
if ($contentLength !== 1) {
$error = 'Language constructs must be followed by a single space; expected 1 space but found %s';
$data = array($contentLength);
$content = $tokens[($stackPtr + 1)]['content'];
if ($content !== ' ') {
$error = 'Language constructs must be followed by a single space; expected 1 space but found "%s"';
$data = array(Util\Common::prepareForOutput($content));
$fix = $phpcsFile->addFixableError($error, $stackPtr, 'IncorrectSingle', $data);
if ($fix === true) {
$phpcsFile->fixer->replaceToken(($stackPtr + 1), ' ');
Expand Down
7 changes: 7 additions & 0 deletions src/Standards/Squiz/Tests/WhiteSpace/LanguageConstructSpacingUnitTest.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,10 @@ return;
return $blah;
return $blah;
return($blah);

return $tab;
return
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that this change here will raise a lot of issues. According to my understanding (maybe I am wrong) the goal of this sniff is to ensure that there are no two spaces between language constructs and $variables/functions etc.

So as I understand it the goal is to avoid this:

return  ['test' => 'value'];

and not this:

return  
    [
        'test' => 'value'
    ];

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have right, but sniff description says:

Ensures all language constructs contain a single space between themselves and their content.

And I think that it is exactly what the sniff should do, we didn't have covered that case previously in tests, so I think @gsherwood is gonna decide what to do.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That could be avoided by checking for ' ' or $phpcsFile->eolChar

Copy link
Contributor

@glen-84 glen-84 May 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To work around this problem, you can use:

return (
    [
        'test' => 'value'
    ]
);

Edit: Or just:

return [
    'test' => 'value'
];

But in my case I have stuff like:

return
    $qb->from(Entities\Player::class, 'p')
        ->leftJoin('p.user', 'u')
        ->select('p', 'u')
        ->where('CASE WHEN p.name IS NULL THEN u.username ELSE p.name END = ?1')
        ->setParameter(1, $name)
        ->getQuery()
        ->getOneOrNullResult();

... so the parentheses work better.

$newLine;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this is gonna be merged I believe it should cover one more test case. What happens if there is a single space after return and then a new line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gmponos I've added this test case and it seems to be fine 😄 Please see 5324d96

// The following line must be the last line in the file
return
6 changes: 6 additions & 0 deletions src/Standards/Squiz/Tests/WhiteSpace/LanguageConstructSpacingUnitTest.inc.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,9 @@ return;
return $blah;
return $blah;
return($blah);

return $tab;
return $newLine;

// The following line must be the last line in the file
return
2 changes: 2 additions & 0 deletions src/Standards/Squiz/Tests/WhiteSpace/LanguageConstructSpacingUnitTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ public function getErrorList()
23 => 1,
27 => 1,
31 => 1,
34 => 1,
35 => 1,
);

}//end getErrorList()
Expand Down