Squiz.Arrays.ArrayDeclaration not detecting some arrays with multiple arguments on the same line #2812
Conversation
| $valuePointer = $value['value']; | ||
|
|
||
| if ($tokens[$valuePointer]['line'] === $tokens[$stackPtr]['line']) { | ||
| $error = 'The first value in a multi-value array must be on a new line'; |
There was a problem hiding this comment.
Does this mean the code
$a = array($b, $c, $d,
$e);
Will return three times the same error ?
One time for the value $b, one time for $c and one time for $d.
There was a problem hiding this comment.
I didn't add this code, I only moved it to the more appropriate position. And no, this code deals only with the first element of the array. And in general, afaik, yes - each misplaced element gets its own error. However, I'm not sure as I really didn't focus on that.
There was a problem hiding this comment.
I try my example, and got the error multiple times.
The check $previous === false should be added.
The best should be grouping the two checks for 'Each value in a multi-line array must be on a new line' and 'The first value in a multi-value array must be on a new line'. See next comment
There was a problem hiding this comment.
Good catch with that example. I took inspiration in your next comment but fixed it a little differently. Anyway, it's now simpler and works better :)
|
... so, does someone want to merge this, or...? |
|
It's just a question of time. @gsherwood will take a look when he'll have time. ATM there are conflicts. |
|
Yeah, I don't want to keep fixing conflicts ... please let me know when you are ready for this and I'll look into the conflict. Thanks. |
|
You can close your PR then because I don't think you will be merge that way. |
Also refactor whitespace a little
|
Fixed it. Please make sure it gets merged this time - it's a bug fix, and it was 3 months already... As for:
I was very close to doing so just because you wrote that. I'm not sure that you understand how frustrating it is to have valuable time wasted, and then have it thrown at you like this. |
Do you know that I'm not a maintainer ? And I have at least 3 PR waiting for 3 months or more on this project. I understand the situation. I just said that you should make your PR RTM at any time and ping the good person if you want to increase the chance to be merged and decrease the leadtime. Complaining and asking to hurry without pinging the maintainers is inefficient. |
|
I assumed you are maintainer/someone close, as people who aren't usually don't do code reviews. I don't think that pinging someone is a good strategy - this is how you overload someone's inbox if they even have the notifications turned on. |
Just so you know: People interested in a certain fix often also do code reviews, even when they are not otherwise involved in the project. Same like @VincentLanglet I'm not a maintainer of this repo either, though I contribute frequently. Reason why I've not looked at this PR is simple: the For the standards I'm involved with, we've moved to our own Array sniffs and I'm working on making those even better and more easily available for other people via the |
Whoah ! Just discovered this ! |
|
@jrfnl Thank you for the information. I tried to maintain "backwards compatiblity", thats why I decided to fix the sniff instead just creating a new one ... And yeah, I also had no idea this other repo exists :D I mainly focus on sniffs at https://github.com/slevomat/coding-standard - they have thousand times more better testing enviroment, useful helpers, etc... So ok, I'll just stop using this sniff a migrate the functionality I need somewhere else (or maybe use yours, I'll check it out later :) ) Thanks for the time. @VincentLanglet I bet one of the reasons is that it takes forever to merge anything here :D I also personally think that sniffs shouldn't be part of this repo at all. |
|
@grongor @VincentLanglet I'm hoping to finish the initial set of sniffs for |
More than anything, most of those sniffs use As #2456 didn't get merged, I see no point in duplicating that work in individual sniffs and pulling them here, so that's why I'm publishing new sniffs in |
Sorry. I get a lot of PRs I need to look through and this one is still sitting in the list. Given this is a bug fix, it's often better to include a description of what the problem is, along with sample code, inside the PR description. This makes triage a lot easier and may have helped me tag this one as a bug a lot sooner (I try and get to bugs before features). Thanks for the PR though. I haven't tested it out yet, but I've got it down for the next release. |
|
@gsherwood I though that example in the tests is enough - I'll write proper description next time ;-) Thank you for your time. |
It was absolutely enough for me to confirm this as a bug, but I was obviously busy when this got submitted and triaged it as "look at later" because the problem wasn't right in my face. No problem though - you've done the hard work, I just missed it. |
gsherwood
changed the title
|
Thanks a lot for the fix and sorry it took so long to merge. It will be in 3.5.5. |
No description provided.