squizlabs / PHP_CodeSniffer

PHP_CodeSniffer tokenizes PHP files and detects violations of a defined set of coding standards.
BSD 3-Clause "New" or "Revised" License
10.67k stars 1.48k forks source link

Squiz.Commenting.PostStatementComment fix breaks Allman structures (dev-master) #1567

Open louisl opened 7 years ago

louisl commented 7 years ago

I'm getting a loop here because my AllmanControlSignatureSniff is picking up the brace being on the wrong line after the comment is 'fixed' then trying to fix it.

With Allman structures you wouldn't expect anything other than a brace on a line so maybe it could check that?

phpcbf -p -s ./test.php --standard=Squiz --sniffs=Squiz.Commenting.PostStatementComment

test.php

if ($prefs['format'] === 'zip')
{
    // Set the filename if not provided (only needed with Zip files)
}
elseif ($prefs['format'] === 'txt') // Was a text file requested?
{
    return $this->_backup($prefs);
}
elseif ($prefs['format'] === 'gzip') // Was a Gzip file requested?
{
    return 'foo';
}

Fixed

if ($prefs['format'] === 'zip')
{
    // Set the filename if not provided (only needed with Zip files)
}
elseif ($prefs['format'] === 'txt') 
// Was a text file requested?
{
    return $this->_backup($prefs);
}
elseif ($prefs['format'] === 'gzip') 
// Was a Gzip file requested?
{
    return 'foo';
}

Expected

if ($prefs['format'] === 'zip')
{
    // Set the filename if not provided (only needed with Zip files)
}
elseif ($prefs['format'] === 'txt') 
{
    // Was a text file requested?
    return $this->_backup($prefs);
}
elseif ($prefs['format'] === 'gzip') 
{
    // Was a Gzip file requested?
    return 'foo';
}
louisl commented 7 years ago

With Allman structures you wouldn't expect anything other than a brace on a line so maybe it could check that?

Actually you cant do that in case of this scenario where someones standard might require end comments:

if ($a === $b)
{
    return $b;
} // Check if a = c.
elseif ($a === $c) // Does a = c?
{
    return $c;
} // Check if a = d.
elseif ($a === $d) // Does a = d?
{
    return $d;
} // end if

I think it might require a custom sniff which disallows comments on brace lines for Allman structures.

I've switched this to a warning in my ruleset for now while I think about it.

<rule ref="Squiz.Commenting.PostStatementComment">
        <type>warning</type>
        <exclude phpcbf-only="true" name="Squiz.Commenting.PostStatementComment.Found"/>
</rule>