Squiz.WhiteSpace.OperatorSpacing false positives for some negation operators #2640
Conversation
The tests only consider this sniff and this sniff alone. If the fixer is being invoked on code where it shouldn't be during the unit tests, I suspect there's still something wrong with this PR. |
src/Standards/Squiz/Tests/WhiteSpace/OperatorSpacingUnitTest.inc
Outdated
Show resolved
Hide resolved
Ok, I'll look into it in more detail ... |
|
@jrfnl you were, there was a bug :) I was juggling code between 3 code style repositories all day today, so I missed it somehow :D thx ... now it should be alright :) |
gsherwood
changed the title
|
Thanks a lot for these changes. This fix will be in 3.5.3. |
PR squizlabs#2640 made it so the `Squiz.WhiteSpace.OperatorSpacing` sniff now sets a property with the `$nonOperandTokens` from the `register()` method. However, the PSR12 sniff for the same extends the Squiz sniff, but overloads the `register()` method to set different targets. This means that the `$nonOperandTokens` array was empty and therefore the `isOperator()` method would nearly always return `true`, which was often incorrect. Fixed by calling the `parent::register()` method before setting the `$targets` for the PSR12 sniff. Includes adding unit tests to the PSR12 sniff related to the exclusions handled via the `isOperator()` method, to prevent breakage like this from being able to be merged in the future.
Not all tokens were handled, so I added them + refactored the code a bit.
I also added a bunch of test code to test all the possible (hopefully) variants.
On a side note: I think that the "fix tests" are somewhat broken. I would expect that the code I added won't be altered at all during the fixing process (because the sniffs should ignore it completely) but it seems that some other sniffs "fixes it" - is that right? Is that expected behaviour?