CS: trailing commas

[keradus]

Q	A
Branch?	7.1
Bug fix?	no
New feature?	no
Deprecations?	no
Issues	CS
License	MIT

kind-of requested in #53230 (comment)
cc @OskarStark

The diff may be bigger and tricky. I put each part of standalone change as separated commit, easy to revert if you don't like particular change.

This PR is more demonstration what we can do with Fixer, you decides which way to go

[kbond]

In general, I'm in favor of this (and adding to @Symfony ruleset).

[fabpot]

I'm supportive as well (including it in @symfony by default makes sense to me as well).

[OskarStark]

Great idea 😍

[smnandre]

If i understand it correctly, any multiline argument given to a method will get a trailing coma, even if there is only one argument allowed in the method ?

Imaginary code, but i feel some similar are in the changed files

$foobar = 'foobar';
$foobar = ucfirst(
    str_replace(
        ['foo', 'bar'],
        ['Foo', 'Bar'],
        $foobar,
+    ),
);

[keradus]

In general, I'm in favor of this (and adding to @symfony ruleset).

overall, as code-change is big, i want to see the impact on codebase first, ensure the config we want to go with, and when this will be aligned, adjust the ruleset. otherwise we adjust the ruleset and then potentially see not all changes are welcome only afterwards 😅

[keradus]

If i understand it correctly, any multiline argument given to a method will get a trailing coma, even if there is only one argument allowed in the method ?

yes, if opening ( and closing ) are in different lines, arguments will get trailing ,.

[keradus]

had to do some partial revert, looks like we found bug on Pslam
551c5f3

[smnandre]

I'm not convinced this is an improvement in term of readability.. but that's very personal :)

$foo = my_method([
    'foo' => 'foo',
    'bar' => 'Bar',
-]);
+],);

[kbond]

I'm not convinced this is an improvement in term of readability

You're example wouldn't be adjusted this way (from what I can see in my app).

[smnandre]

I'm not convinced this is an improvement in term of readability

You're example wouldn't be adjusted this way (from what I can see in my app).

Indeed, thanks.

I over-generalized.. the "problematic" one for me would be the one i just quoted : method with a single argument.

[keradus]

not sure if failing related anyhow? happy to get guidance

https://github.com/symfony/symfony/actions/runs/7390078815/job/20104252552?pr=53352#step:8:8085

[nicolas-grekas]

Is there a way to tweak the rule to require the comma on the declaration side, but not on the call side?

[keradus]

If I understood right, you want elements => parameters to be enabled, but elements => arguments to be disabled.

We have this tech possibility and I would encourage to have both enabled, to make the rule simpler ("trailing commas everywhere" rather than "trailing commas here and here, but not there"). Would you mind to share your reasoning for idea to not enable everywhere, so I can understand better?

Yet, if you are concluding that you want to disable trailing commas on arguments, I can do it - it's already better for me to enable it in some places that in none of them.

[derrabus]

Is there a way to tweak the rule to require the comma on the declaration side, but not on the call side?

I would want them on both sides, tbh.

[nicolas-grekas]

I want trailing commas on constructor declarations (and on arrays) because it's quite common to add a new argument there and having to add a comma after the last existing argument creates noisy diffs that complicate reviews, blurs the git history, etc.

But I'd prefer no trailing comma on function calls because a trailing comma tells my brain something can come next, while it's usually not the case (str_replace($a, $b, $c,)) => "what could be after "$c"??? 🧠 )

This breaks my brain as @chalasr wrote somewhere on another CS topic :)

[derrabus]

You can train your brain to accept this. Trust me. ✌️

[keradus]

I use it myself and it hurts me to not have have trailing comma 😅 all is a matter of habit.
Consider having function like foo($a, $b=2, $c=3, $d=4, ...$args=[]). Then you see trailing commas for function calls make sense too ;)

Happy to go with any direction.
Also, we can split PR and merge everything except method calls and have method calls as separated discussion.

tell me and I will follow.

still, no clue on CI error :(

[fabpot]

I'm all in favor of commas at the end of a line as it minimizes the diff when adding a new line.
But I'm against adding visual clutter (unneeded characters) like in Nicolas example: str_replace($a, $b, $c,). I don't see any benefit. So, it's not that it "hurts my brain", but more like there is no benefit in having this additional character.

[xabbuh]

I agree with Nicolas and Fabien. Having the trailing comma at the end of a line is great because of the reduced diffs, but having it at the end of arguments on on-liners is not so nice.

[nicolas-grekas]

having it at the end of arguments on on-liners is not so nice

on one-liners and multi-liners ;)

[OskarStark]

Exactly, only on multiline

[nicolas-grekas]

@keradus can you please update the PR to add the trailing commas on declaration sites only? At least we have an agreement on that so let's move on. Then if one wants the comma on call sites let's make it another discussion (and I will object like I'm doing here :) )

[keradus]

But I'm against adding visual clutter (unneeded characters) like in Nicolas example: str_replace($a, $b, $c,).

not sure if we talk about same thing. To highlight... with this PR

this will NOT be modified

 str_replace($a, $b, $c);

this will be modified

 str_replace(
    $a,
    $b,
-   $c
+   $c,
  );

[keradus]

@nicolas-grekas , i think all but you are to enable everything.
I am OK to merge some by now and continue discussion about other part of it after this partial merge already,

but please clarify what you want to have, #53352 (comment)

"declaration site" is blurry for me and I'm guessing which you want or want not to have - eg, I understood you would like to have trailing comma in arrays, but I struggle to call array a "declaration site".
Please consider to mark for revert one of commits I mentioned in linked msg or state configuration you are happy with - values in PR , but also overview here: https://cs.symfony.com/doc/rules/control_structure/trailing_comma_in_multiline.html#elements

[nicolas-grekas]

I guess I mean to enable "arguments" but not "parameters"?

eg this is fine:

 function __construct(
    $a,
    $b,
-   $c
+   $c,
  );

and this isn't to me:

 str_replace(
    $a,
    $b,
-   $c
+   $c,
  );

The reason being the visual cluttering of the latter with no justification. It's super rare to add parameters to a call, while it's quite common on a function/method signature (esp for constructors, which is how all this discussion started)

[keradus]

I believe it's as common to add input to function declaration as to function call, as whenever we change one, we change another ;) yeah - maybe constructor of service being good exception here.

I reverted arguments ref and will trigger it as separated PR, after this PR gets merged.

[nicolas-grekas]

Can you please squash to get rid of the merge commit.

Otherwise, that's the perfect diff: small, and exactly the kind of changes we were asking for during reviews. (But we never asked contributors to add trailing commas to multline calls ;) )

[keradus]

I will (in background), but curious - with all new greatest possibility of github and old features of git itself, why not squash during merge? :)

--
rebased

[OskarStark]

I will (in background), but curious - with all new greatest possibility of github and old features of git itself, why not squash during merge? :)

We are using a dedicated tool to merge PRs on symfony/*, while squashing is no problem, we cannot do it if the PR contains a merge commit 🤷‍♂️

[keradus]

@OskarStark I am aware ;) I was mentioning this in context of PRs I did open for that tool (133, 140) in 2018 to solve exactly this problem ;)

unfortunately they did not got enough attention to get merged 😅
I'm not proposing myself for that again, but maybe it's worth to consider having built-in support of merge-commits so we do not need to ask contributors for this anymore. We switched to a custom fork for Fixer and used it over months (now we actually use github button :) ), it helped with experience for contributors to avoid "get rid of merge commit" requests and even eased with the PR reviews (no longer destructive pushes to PRs).

[nicolas-grekas]

Thank you @keradus.

[keradus]

follow-up created as discussed: #53395