Generic/IncrementDecrementSpacing: handle more situations#3626
Generic/IncrementDecrementSpacing: handle more situations#3626jrfnl wants to merge 1 commit intosquizlabs:masterfrom
Conversation
The `Generic.WhiteSpace.IncrementDecrementSpacing` sniff, so far, only handled incrementors/decrementors when they were directly before/after the variable they apply to. This commit enhances the sniff to also allow for finding superfluous whitespace when incrementing/decrementing a property or an array item. Includes unit tests.
2ed3d67 to
80a2d06
Compare
DannyvdSluijs
left a comment
There was a problem hiding this comment.
Seems all to be working okay, just had a small comment about adding test cases for the pre increment instead of only testing post increment.
| $i ++; | ||
| $i /*comment*/ ++; | ||
|
|
||
| // Handle properties and array access too. |
There was a problem hiding this comment.
Should we expand the test case with pre-increments of the array and object properties?
++$i['key'];
++ $i['key'];
++$i['key']['id'];
++ $i['key']['id'];
++$obj->prop;
++ $obj->prop;
++ $obj?->prop;
++$obj->obj->prop;
++ $obj->obj->prop;
++ $obj?->obj->prop;
++$obj->prop['key'];
++ $obj->prop['key'];There was a problem hiding this comment.
@DannyvdSluijs Good question. In my opinion: no, those tests are not needed as they wouldn't be testing anything (new).
The difference in this case for pre vs post in/decrement is that to identify whether something is a "pre" vs "post" increment (i.e. to identify whether there should be no whitespace after or no whitespace before, the sniff needs to look at different tokens. These extra tests cover the changes I made to that logic.
Adding the same tests for pre-increment wouldn't actually test anything new as it was already handled correctly (as whether something is a plain variable or a non-static property access or array key access on a variable doesn't make a difference in the identification of pre vs post).
For pre-increment, I can think of a further/future iteration for the sniff - checking whether a pre-increment is used on a static property with a fully qualified classname or namespace relative classname, but that is something I choose not to handle (yet) when I made this change last year. When that change would be added, then, yes, extra tests would be needed for pre-in/decrement.
++\ClassName::$prop;
++Relative\ClassName::$prop;
--namespace\Relative\ClassName::$prop;The reason I did not make that change (yet) is that this would need a different patch for PHPCS 3.x vs PHPCS 4.x, which would make the merge more complex. Also see #3041.
There was a problem hiding this comment.
That all makes sense to me. Which makes this PR ready to merge as far as I'm concerned.
|
Closing as replaced by PHPCSStandards/PHP_CodeSniffer#46 |
|
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). |
The
Generic.WhiteSpace.IncrementDecrementSpacingsniff, so far, only handled incrementors/decrementors when they were directly before/after the variable they apply to.This commit enhances the sniff to also allow for finding superfluous whitespace when incrementing/decrementing a property or an array item.
Includes unit tests.
Note: I've only made this change for PHP files as JS support will be dropped anyway, so it didn't feel like a good use of my time to work on that.