Jump to content

Topic on Manual talk:Coding conventions/Flow

Trailing commas in PHP arrays

7
Tgr (WMF) (talkcontribs)

Per T222042, it's possible now to enforce trailing commas in the last lines of multiline PHP arrays (with phpcbf including an automatic fix). I propose we enable it by default (add it to the MediaWiki style):

  • it makes code style less ambiguous (which is generally a good thing, because there is less room for reviewer subjectivity, and less room for different repos ending up with different style checks);
  • it simplifies diffs (only the line you added shows up as a change and not the previous one) and reduces the chance of merge conflicts;
  • it makes adding and removing lines at the end of arrays more convenient and less error-prone.
Krinkle (talkcontribs)

LGTM. I support enablement (provided it is auto-fixed) of requiring trailing commas in multi-line arrays.

I find it valuable to remove a choice that shouldn't carry any meaning in the first place (namely, the presence or absence of a trailing comma). Specifically, the choice to prefer presence (instead of absence) of trailing commas I believe improves diffs, as well as eases refactoring when re-ordering or copying code from one place to another.

With regard to Thiemo's point, about communicating the significance of a final item through absence of a trailing comma: I do not consider the presence or absence of a comma to be effective communication between developers. I think this is too subtle. When code has a limited-length return value, it should be described in its doc block, and/or covered by strictly asserting unit test, and/or typed with value shape to Phan, or if otherwise important through an inline comment. See also: omitting the (technically optional) semi-colon after the last property of a CSS rule, or after the last statement in a JavaScript function.

We can either: make the trailing symbol redundantly reflect a pre-existing semantic and train ourselves to remember to type that correctly — or, — we can make it a consistent part of mental boilerplate and remove the part of our brain that will draw attention to this detail every time we read a line of code to determine whether and what it could or should mean.

Thiemo Kreuz (WMDE) (talkcontribs)

Seeing my argument being dissected like this is … weird. Just because the absence of a comma isn't communicating much doesn't imply the opposite – that we must enforce it everywhere. I could start listing arguments again – like that I seriously don't believe such a rule would have any impact on the mental effort required to read diffs. But my point is: I also find the extra comma helpful and almost always add it. I just don't think it's worth bothering anybody with a failing CI job. 😢️

This post was hidden by Thiemo Kreuz (WMDE) (history)
MusikAnimal (talkcontribs)

+1! I have long wanted this rule to be present in our styles, and in fact (at least for JavaScript, think?) the opposite is enforced, which I find frustrating. I suggest we enforce comma-dangle for JS as well, for the same reasons we're doing it for PHP. On that note, there seems to be a lot of stylistic crossover between the two languages and I wish we made more effort to keep the code styles consistent.

Tim Starling (WMF) (talkcontribs)

Nitpicking wastes time whether it is automated or manual. I don't support making this change in core or any other repo. I would support removing the sniff from Wikibase.

Tgr (talkcontribs)

FWIW trailing commas are now mandated by PER-CS (the PSR group's attempt at an official PHP code style).

Reply to "Trailing commas in PHP arrays"