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.

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

ControlStructureSpacing sniff: fix blank line after control structure / disappearing comment #981

Merged
merged 3 commits into from Jun 20, 2017

Conversation

@jrfnl
Contributor

jrfnl commented Jun 18, 2017

This bug was introduced in 786c9bb but had so far gone unreported. This PR effectively reverts the offending part of that commit.

Fixes #976

@jrfnl jrfnl added this to the 0.12.0 milestone Jun 18, 2017

Show outdated Hide outdated WordPress/Sniffs/WhiteSpace/ControlStructureSpacingSniff.php
@@ -496,7 +496,8 @@ public function process_token( $stackPtr ) {
return;
}
$trailingContent = $this->phpcsFile->findNext( PHP_CodeSniffer_Tokens::$emptyTokens, ( $scopeCloser + 1 ), null, true );
// {@internal This is just for the blank line check. Only whitespace should be considered.}}

This comment has been minimized.

@GaryJones

GaryJones Jun 18, 2017

Member

Is the internal annotation required? This is not a DocBlock.

@GaryJones

GaryJones Jun 18, 2017

Member

Is the internal annotation required? This is not a DocBlock.

This comment has been minimized.

@jrfnl

jrfnl Jun 18, 2017

Contributor

Not required, but felt like the appropriate annotation. It's mostly to prevent me from changing it back when doing spot checks whether the appropriate token is used 😇

@jrfnl

jrfnl Jun 18, 2017

Contributor

Not required, but felt like the appropriate annotation. It's mostly to prevent me from changing it back when doing spot checks whether the appropriate token is used 😇

This comment has been minimized.

@JDGrimes

JDGrimes Jun 19, 2017

Contributor

Could add "not other empty tokens" to make it more clear.

@JDGrimes

JDGrimes Jun 19, 2017

Contributor

Could add "not other empty tokens" to make it more clear.

Show outdated Hide outdated WordPress/Sniffs/WhiteSpace/ControlStructureSpacingSniff.php
@@ -506,6 +507,16 @@ public function process_token( $stackPtr ) {
return;
}
if ( T_COMMENT === $this->tokens[ $trailingContent ]['code'] ) {
if ( $this->tokens[ $trailingContent ]['line'] === $this->tokens[ $scopeCloser ]['line'] ) {
if ( '//end' === substr( $this->tokens[ $trailingContent ]['content'], 0, 5 ) ) {

This comment has been minimized.

@JDGrimes

JDGrimes Jun 19, 2017

Contributor

What about // end, // End, etc. ? Does it really even matter what the comment looks like here? Would be good to see some tests that show what this part does and that it works correctly.

@JDGrimes

JDGrimes Jun 19, 2017

Contributor

What about // end, // End, etc. ? Does it really even matter what the comment looks like here? Would be good to see some tests that show what this part does and that it works correctly.

This comment has been minimized.

@jrfnl

jrfnl Jun 19, 2017

Contributor

Just ran some tests and interestingly enough, that causes yet another "case of the disappearing comment" similar to #976

Looking at the history, this sniff was original cloned from the Squiz standard and the fixers were build in afterwards in 2014. Since the original cloning, the upstream sniff also gained fixers.
I know I've synced a number of relevant changes from upstream in before, but don't think that part has changed that much. Will have a look and see what we can learn from the upstream version.

@jrfnl

jrfnl Jun 19, 2017

Contributor

Just ran some tests and interestingly enough, that causes yet another "case of the disappearing comment" similar to #976

Looking at the history, this sniff was original cloned from the Squiz standard and the fixers were build in afterwards in 2014. Since the original cloning, the upstream sniff also gained fixers.
I know I've synced a number of relevant changes from upstream in before, but don't think that part has changed that much. Will have a look and see what we can learn from the upstream version.

ControlStructureSpacing sniff: Sync in more changes from upstream for…
… the "blank line after" check

The upstream sniff is *not* properly unit tested with a fixed file and in part checks for something different and therefore uses different logic.

This commit adds unit tests for our version of the sniff, syncs in a number of good changes from upstream and fixes a number of bugs related to this particular check & fixer.
@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Jun 19, 2017

Contributor

I've synced in a number of snippets from upstream & fixed the second disappearing comment bug. Fingers crossed, this should be good now.

Contributor

jrfnl commented Jun 19, 2017

I've synced in a number of snippets from upstream & fixed the second disappearing comment bug. Fingers crossed, this should be good now.

@JDGrimes JDGrimes merged commit b69f345 into develop Jun 20, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@JDGrimes JDGrimes deleted the feature/issue-976-disappearing-comment branch Jun 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment