-
-
Notifications
You must be signed in to change notification settings - Fork 57
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
Array destructuring in foreach - conflict between SpaceBeforeComma & NoSpaceAfterComma #527
Comments
@VladaHejda Thank you for reporting this. This is due to the sniff incorrectly acting on short lists. The I've been building up a (highly configurable) This specific case does sound like a relatively simple fix though, so if someone would submit a PR with a small/simple fix for this specific issue, I will accept it, but other than that, this sniff is out of bounds for large fixes/rewrites as it is just not worth the time. |
@jrfnl, I spent some time checking what is causing this issue and how to fix it. Initially, I thought the idea was to bail early when the If the above is correct and I'm not missing something, the other solution that occurred to me is to bail early when the
Is this more or less what you had in mind? Thanks! |
Correct. Filtering out short lists completely for this sniff is not on the table due to the complexity.
That was the idea yes, but there is a scenario when a foreach ([$a, $b] as $c) {}
Sort of... Note: even with this additional check, this can still lead to false positives/negatives (short list as a value in an array before the Does this help ? |
Describe the bug
Destructuring an array for foreach -
foreach ($data as [, , $value]) {
- violates theSquiz.Arrays.ArrayDeclaration.SpaceBeforeComma
("Expected 0 spaces between "," and comma; 1 found")But when "fixed" -
foreach ($data as [,, $value]) {
- theSquiz.Arrays.ArrayDeclaration.NoSpaceAfterComma
is violated ("Expected 1 space between comma and ","; 0 found")So this code cannot be written while using both sniffs together.
Code sample
Versions
The text was updated successfully, but these errors were encountered: