feat: multiline long array

[bachinblack]

Fixes #1217

---

This is my very first PR on an open source project so please be patient 😄
Also, it's possible that my PR is a duplicate of an already existing fixer, in which case I'll probably cry.

This PR is a possible solution to #4485 (issue I stumbled upon while looking for such a fixer).

It formats arrays according to the max_length argument (which is the maximum authorized length for a single line array).

Why I think it's good.

I recently used rector to upgrade my code and it turned my attributes into gigantic one-liners. Definitely not fixing them by hand.
Apart from that, I could use some logic on my decision-making regarding array formats.

Scope

1. The max_length argument is defined in characters and excludes whitespaces. Another possible option would be to count the number of items (not for me, though).

2. In the issue (Single line array to multiple line array fixer #4485) it is also proposed to make an array multiline if it contains key/value pairs. To me it's not a great option as the type of an array hardly relates to its size but it's only fair to mention it.

3. It only works with arrays but it could also be useful for function arguments and chained method calls.
   Possibly in the same fixer, given extra configuration (the code would be very close), possibly in separate fixers.

Fixer priority (50)

This fixer leans on ArrayIndentationFixer, WhitespaceAfterCommaInArrayFixer, NoWhitespaceBeforeCommaInArrayFixer,TrailingCommaInMultilineFixer and NoTrailingCommaInSinglelineFixer to output nicely formatted code. It contains no logic at all regarding indentation or commas, is it okay ? Or should I also add this kind of logic to it ? Feels like duplication so I guess not.

Thanks for your time !

[bachinblack]

Fixed most of the problems with the CI but I can't seem to find how to test getPriority(). Can anyone help me with this ?

[Wirone]

@bachinblack hi, thank you for the contribution. From top of my head I can't tell right now if we already had such a fixer, but I'm on vacation now and can look deeper later (Thursday+). Anyway, I accepted workflow so you'll get CI feedback. In terms of testing priority you need integration test like this.

PS. Please don't cry if it doesn't get merged, it's nice knowledge anyway (what you have learned during the process) 😊.

[bachinblack]

Hey ! Thanks for your help, I managed to add integration tests for my fixer.
I think it would be nice to add a section in the cookbook about priority tests. Maybe a subject for another PR ?
Anyway, it should be ready for a review and I'm open to input.

[Wirone]

FYI: there is issue #6225 and there was PR #6387 that addresses same concept.

[bachinblack]

This fixer should also come before binary_operator_spaces.

[lugosium]

Looks great @bachinblack 🎉 Thanks for your time 👍

Any news on this feature?

[Wirone]

I believe fixer should react also to CT::T_DESTRUCTURING_SQUARE_BRACE_OPEN and handle code like:

// Before
[$foo, $bar, $baz] = $arr;

// After
[
    $foo,
    $bar,
    $baz
] = $arr;

[bachinblack]

Makes sense. I added the logic and I made a test for it, does it work for you ?

[Wirone]

Please add such case to tests/Fixtures/Integration/priority/multiline_long_array,array_indentation.test too, so we can be sure that it works as expected 🙂.

[bachinblack]

Will try to do it when I find some time this week

[bachinblack]

I just added the test but I'm not sure what it was supposed to do.

I was expecting :

// from
[$foo, $bar, $baz] = $arr;

// to
[
    $foo,
    $bar,
    $baz
] = $arr;

But I got :

[
$foo,
$bar,
$baz
] = $arr;

After looking around, it seems like ArrayIndentation does not work with destructuring, right ?
So is this what was expected ? If not, is the test still relevant ?

[Wirone]

It'll work when #7405 is merged, so we need to update integration test here when it's done 🙂.

[bachinblack]

Got you 👍 I just added the test

[Wirone]

@bachinblack in general it looks OK for me, but we had discussion about it with @kubawerlos and we think @SpacePossum's approach in #6387 was interesting, to have 2 separate conditions for triggering rule and strategy to choose (at least one, both). Your PR is focused on array length, but can be extended later with new options - are you interested in this? If yes, would you prefer to do it in this PR or later in second one? Of course we can discuss everything before so the outcome is as expected.

[bachinblack]

Hey @Wirone ! I wouldn't use this rule with an elements count myself because I don't think it's as reliable as a characters count but I guess it would be normal to give users the choice and it doesn't seem that hard to implement (though I believe there will be many more tests).

For the param names, I can get closer to @SpacePossum 's naming elements_threshold/characters_threshold and make them both nullable to let users pick what they want.

I think I'd be more comfortable merging it as it is now (after having renamed the current param to characters_threshold) and starting a new one after if that's alright.

[bachinblack]

As I proposed earlier, I rename max_length to characters_threshold so that it's clearer and it can easily be extended later with elements_threshold.

I understand fixers' default configuration can't just do nothing and there has to be a default config, so I chose to keep the default behaviour to always multiline.

Now, should we add elements_threshold, we'd need to keep this default behaviour.

Re-review required, previous comments were addressed

[bachinblack]

Hey ! I've caught up with master so it should be all good now, would anyone be so kind as to review my PR once more ? (hopefully it will be the last time haha)

[jklmnop]

This looks great seems like just what I'm looking for! Hoping it gets merged into release. 🤞

[mkroener]

I'm hoping this gets merged too as the current array formatting capabilities prevent me from fully integrating php-cs-fixer into my project.
If this is is abandoned and there is still interest in this I would be happy to work on this to get it merged

[jlecordier]

Mhh, I guess that's subjective then, thanks for answering anyway !