Squiz/OperatorSpacing: bug fix - prevent fixer conflict #427
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fredden
approved these changes
Apr 9, 2024
Another one in the fixer conflict series.
When running the `Squiz` standard over all test case files, a fixer conflict in the `Squiz.WhiteSpace.OperatorSpacing` sniff was discovered via the tests in the `./src/Standards/Generic/Tests/CodeAnalysis/RequireExplicitBooleanOperatorPrecedenceUnitTest.inc` file for a code sample like this:
```php
$foo = $var // Comment.
? false // Comment.
: true;
```
The conflict essentially comes down to the `Squiz.WhiteSpace.OperatorSpacing` trying to remove the new line before the `?` and the `:` operator, but failing to do so as the new line is included in the comment token on the previous line and the sniff only adjusts/removes whitespace tokens.
Original errors for the code sample added in the test case file:
```
487 | ERROR | [x] Expected 1 space before "?"; newline found (Squiz.WhiteSpace.OperatorSpacing.SpacingBefore)
488 | ERROR | [x] Expected 1 space before ":"; newline found (Squiz.WhiteSpace.OperatorSpacing.SpacingBefore)
493 | ERROR | [x] Expected 1 space before "?"; newline found (Squiz.WhiteSpace.OperatorSpacing.SpacingBefore)
494 | ERROR | [x] Expected 1 space before ":"; newline found (Squiz.WhiteSpace.OperatorSpacing.SpacingBefore)
499 | ERROR | [x] Expected 1 space before "?"; newline found (Squiz.WhiteSpace.OperatorSpacing.SpacingBefore)
504 | ERROR | [x] Expected 1 space before ":"; newline found (Squiz.WhiteSpace.OperatorSpacing.SpacingBefore)
```
The fix proposed in this PR changes the sniff to make the `SpacingBefore` error code non-fixable if the previous non-whitespace token is a comment token - even when the comment token doesn't contain a new line.
While the sniff _could_ auto-fix when the comment token doesn't contain a new line, I have chosen to disable the auto-fixer for those cases anyway as it is not for the sniff to determine whether the comment should be moved, removed or should stay where it is.
With these changes, the code sample in the test case file now yields the following errors:
```
487 | ERROR | [x] Expected 1 space before "?"; newline found (Squiz.WhiteSpace.OperatorSpacing.SpacingBefore)
488 | ERROR | [x] Expected 1 space before ":"; newline found (Squiz.WhiteSpace.OperatorSpacing.SpacingBefore)
493 | ERROR | [ ] Expected 1 space before "?"; newline found (Squiz.WhiteSpace.OperatorSpacing.SpacingBefore)
494 | ERROR | [ ] Expected 1 space before ":"; newline found (Squiz.WhiteSpace.OperatorSpacing.SpacingBefore)
499 | ERROR | [ ] Expected 1 space before "?"; newline found (Squiz.WhiteSpace.OperatorSpacing.SpacingBefore)
504 | ERROR | [ ] Expected 1 space before ":"; newline found (Squiz.WhiteSpace.OperatorSpacing.SpacingBefore)
```
This effectively fixes the fixer conflict.
:point_right: The diff will be easier to review while ignoring whitespace changes.
---
<details>
<summary>Fixer Conflict details</summary>
```
* fixed 0 violations, starting loop 48 *
=> Changeset started by Squiz.WhiteSpace.OperatorSpacing:258
Q: Squiz.WhiteSpace.OperatorSpacing:267 replaced token 911 (T_WHITESPACE on line 72) " ?" => " ?"
A: Squiz.WhiteSpace.OperatorSpacing:268 replaced token 911 (T_WHITESPACE on line 72) " ?" => " ?"
=> Changeset ended: 1 changes applied
=> Changeset started by Squiz.WhiteSpace.OperatorSpacing:258
Q: Squiz.WhiteSpace.OperatorSpacing:267 replaced token 925 (T_WHITESPACE on line 73) " :" => " :"
A: Squiz.WhiteSpace.OperatorSpacing:268 replaced token 925 (T_WHITESPACE on line 73) " :" => " :"
=> Changeset ended: 1 changes applied
=> Fixing file: 2/2 violations remaining [made 48 passes]...
* fixed 2 violations, starting loop 49 *
=> Changeset started by Squiz.WhiteSpace.OperatorSpacing:258
Q: Squiz.WhiteSpace.OperatorSpacing:267 replaced token 911 (T_WHITESPACE on line 72) " ?" => " ?"
**** Squiz.WhiteSpace.OperatorSpacing:268 has possible conflict with another sniff on loop 47; caused by the following change ****
**** replaced token 911 (T_WHITESPACE on line 72) " ?" => " ?" ****
**** ignoring all changes until next loop ****
=> Changeset failed to apply
=> Fixing file: 0/2 violations remaining [made 49 passes]...
```
</details>
---
Related to 152
jrfnl
force-pushed
the
feature/squiz-operatorspacing-prevent-fixer-conflict
branch
from
1770f36 to
195e941
Compare
|
Rebased without changes. Merging once the build passes. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Squiz/OperatorSpacing: move parse error test to own test case file
Squiz/OperatorSpacing: bug fix - prevent fixer conflict
Another one in the fixer conflict series.
When running the
Squizstandard over all test case files, a fixer conflict in theSquiz.WhiteSpace.OperatorSpacingsniff was discovered via the tests in the./src/Standards/Generic/Tests/CodeAnalysis/RequireExplicitBooleanOperatorPrecedenceUnitTest.incfile for a code sample like this:The conflict essentially comes down to the
Squiz.WhiteSpace.OperatorSpacingtrying to remove the new line before the?and the:operator, but failing to do so as the new line is included in the comment token on the previous line and the sniff only adjusts/removes whitespace tokens.Original errors for the code sample added in the test case file:
The fix proposed in this PR changes the sniff to make the
SpacingBeforeerror code non-fixable if the previous non-whitespace token is a comment token - even when the comment token doesn't contain a new line.While the sniff could auto-fix when the comment token doesn't contain a new line, I have chosen to disable the auto-fixer for those cases anyway as it is not for the sniff to determine whether the comment should be moved, removed or should stay where it is.
With these changes, the code sample in the test case file now yields the following errors:
This effectively fixes the fixer conflict.
👉 The diff will be easier to review while ignoring whitespace changes.
Fixer Conflict details
Suggested changelog entry
Fixed Squiz.WhiteSpace.OperatorSpacing : when there was a new line before an operator, but the line before it ended on a comment, the fixer would get into a conflict state.
Related issues/external references
Related to #143
Related to #152
Types of changes