Generic/EmptyPHPStatement: improve code coverage #699
Conversation
|
Please move the question regarding the behaviour with short open tags to its own issue (linking to this PR). Different decision point, so it doesn't belong in this PR. |
src/Standards/Generic/Tests/CodeAnalysis/EmptyPHPStatementUnitTest.2.inc
Outdated
Show resolved
Hide resolved
|
|
Thanks for reviewing this PR, @jrfnl! I implemented everything you suggested, and this is now ready for a second review. I split my changes into three different commits as I believe they should be squashed into three different commits before this PR is merged. My suggestion is to combine the second commit that removes an unreachable condition with d23d5ae and edit its commit message to reflect this change.
I think it is a good idea. There is already a test with two superfluous semi-colons after a class definition (src/Standards/Generic/Tests/CodeAnalysis/EmptyPHPStatementUnitTest.1.inc#L63), so I updated other existing tests to include a test with two superfluous semi-colons between PHP tags and a test with two superfluous semi-colons between PHP tags and after a statement. |
rodrigoprimo
force-pushed
the
test-coverage-empty-php-statement
branch
from
428c88c to
ed6233e
Compare
|
@jrfnl, just FYI, the Windows build failed, and I don't think the problem is related to the changes in the PR: https://github.com/PHPCSStandards/PHP_CodeSniffer/actions/runs/12016963721/job/33498304890?pr=699 It seems it was a Coveralls hiccup: https://github.com/PHPCSStandards/PHP_CodeSniffer/actions/runs/12016963721/job/33498304890?pr=699#step:19:193 |
|
I believe you ran the build again while I was typing the previous comment and it passed. Thanks! |
There was a problem hiding this comment.
Thanks @rodrigoprimo! I'll rebase & squash select commits and will merge this PR once the build has passed.
* Removed two unreachable conditions.
`$prevNonEmpty` will never be `false` as there will always be
at least a PHP open tag token before the semicolon token.
* Removed unreachable default case
`$tokens[$stackPtr]['type']` will always be either `T_SEMICOLON` or
`T_CLOSE_TAG` as those are the two tokens that this sniff listens for.
So, the default case will never be reached.
Doing this to be able to create another test file to test the sniff with the short open tag.
* Updated tests to have more examples with more than one superfluous semicolon. * Minor test comment punctuation tweaks.
jrfnl
force-pushed
the
test-coverage-empty-php-statement
branch
from
ed6233e to
eb7c3a9
Compare
Follow up on #699 Commit 529bcba removed the unreachable `default` case from the `switch`, but even when it is not explicitly there, this still leaves an unreachable branch in the code flow. This commit takes the previous change one step further and removes the `switch` completely in favour of two separate `private` functions which each handle one specific token. N.B.: this commit will be easier to review while ignoring whitespace changes.
Follow up on #699 Commit 529bcba removed the unreachable `default` case from the `switch`, but even when it is not explicitly there, this still leaves an unreachable branch in the code flow. This commit takes the previous change one step further and removes the `switch` completely in favour of two separate `private` functions which each handle one specific token. N.B.: this commit will be easier to review while ignoring whitespace changes.
Description
This PR improves code coverage for the
Generic.CodeAnalysis.EmptyPHPStatementsniff.While adding tests for the short open tag, I noticed a behavior in the sniff related to #593 that we might want to address in this PR or in a separate PR if it is not intentional behavior. The sniff fixes<? ; something_else(); ?>to<?something_else(); ?>. Note that there is no space between the PHP open tag and the function call. Should the sniff change to preserve one space between the PHP open tag and the function call? It is probably not a high priority as the resulting code is not a parse error, and short open tags are not very popular.Update: question above moved to #728
Related issues/external references
Part of #146
Types of changes
PR checklist