Auto-fix PEAR.Commenting.FunctionComment.SpacingAfter

[fredden]

Description

I have witnessed some complaints coming from Squiz.Commenting.FunctionComment.SpacingAfter which look like they could be trivially auto-fixed. This pull request adds auto-fixing for this particular complaint. I intend to investigate feasibility of auto-fixing other cases in this sniff another time.

Suggested changelog entry

Allow auto-fixing ...FunctionComment.SpacingAfter

Types of changes

• New feature (non-breaking change which adds functionality)

PR checklist

• I have checked there is no other PR open for the same change.
• I have read the Contribution Guidelines.
• I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
• I have added tests to cover my changes.
• I have verified that the code complies with the projects coding standards.

[jrfnl]

@fredden Thanks for proposing this change. The principle of the change makes sense to me, however not so, the implementation.

Consider the following code:

/**
 * Has space after
 */




function foo() {}

With the proposed change, the fixer would need 7 passes before the issue is fixed as each time only one line is removed + the fixer will skip a few passes due to a suspicion of a conflict.
You can check this yourself by running the fixer with -vvv.

So, instead of the currently proposed change, the implementation should remove all blank lines between the comment closer and the line containing the function keyword in one go.

---

For the record: this change will affect the following sniffs:

• PEAR.Commenting.FunctionComment (sniff in which the change is being made)
• Squiz.Commenting.FunctionComment (extends the PEAR sniff)
• MySource.Commenting.FunctionComment (extends the PEAR sniff)

[fredden]

So, instead of the currently proposed change, the implementation should remove all blank lines between the comment closer and the line containing the function keyword in one go.

Yes, this makes sense. I'll look at making this change in the coming days.

[jrfnl]

@fredden Thanks for updating the patch, but this is still not correct.

With the patch as is, if any of the blank lines between the comment closer and the first code has spaces/tabs on them (still a blank line), the fixer will now go into conflict state.

Try it by adding some spaces on one of the lines in the code sample I provided and see the "FAILED TO FIX" appear.

The test I provided should also be added.

[fredden]

Thanks for the feedback @jrfnl.

With the patch as is, if any of the blank lines between the comment closer and the first code has spaces/tabs on them (still a blank line), the fixer will now go into conflict state.

I did consider the case where there are lines with only spaces/tabs. I thought that another sniff (like Squiz.WhiteSpace.SuperfluousWhitespace) should handle removing trailing white-space from such lines. I did not however check that this sniff would not produce a "failed to fix" situation in that case.

I have updated the code to treat lines containing only spaces/tabs as blank too. I'm now using exactly the same condition/logic to determine if a line is "blank" in the fixer as was used to identify the issue and add the error.

I have added tests to cover these scenarios.

[jrfnl]

@fredden I haven't looked at the complete updated patch yet, just the change in the sniff.

Getting closer, but as the fixer now potentially touches more than one token, the fixer block should be wrapped within a changeset as otherwise the fixer can not correctly revert changes when it detects conflicts.

if ( $fix === true ) {
    $phpcsFile->fixer->beginChangeset();
    ...
    $phpcsFile->fixer->endChangeset();
}

[jrfnl]

@fredden Thanks for making those updates. Looks like this is now working correctly, including correct handling of attributes (not removing the indent whitespace). ✅

[jrfnl]

FYI: this fix is included in today's PHP_CodeSniffer 3.8.0 release.

As per #3932, development on PHP_CodeSniffer will continue in the PHPCSStandards/PHP_CodeSniffer repository. If you want to stay informed, you may want to start "watching" that repo (or watching releases from that repo).