Page MenuHomePhabricator

Require trailing commas in *long* multi-line arrays, function calls and function param lists in MediaWiki code
Closed, DeclinedPublic

Description

Trailing commas in multi-line arrays, function calls and function param lists avoid dirty diffs when adding yet another item or parameter. This happens all the time, especially with things like constructors of service classes, arrays of service parameters for the aforementioned, lists of fields for HTMLForm or their options, lists of attributes for Html::element(), internal API calls, and so on. On the other hand, it is sometimes aesthetically displeasing when an array is technically multiline, but maybe it will only ever have 1 item, or maybe it is technically multiline, but really it's a single line of code that had to be wrapped due to line length.

PHPCS comes with two rules to require trailing commas in multi-line arrays, Squiz.Arrays.ArrayDeclaration.NoCommaAfterLast, Squiz.Arrays.ArrayDeclaration.NoComma (despite the names, they both require commas to be present after the last array elements, but one applies to associative arrays and the other one to numeric arrays), but they are not nuanced like that, and I didn't find anything similar for function calls (trailing commas allowed since PHP 7.3) and parameter definitions (trailing commas allowed since PHP 8.0).

Maybe we can come up with something with a light touch that will enforce this in the obvious cases where it helps, but doesn't disturb the code in the less obvious cases. Something like: only require the trailing comma when the array/call/definition is multi-line, each item is on a separate line (so it's not a wrapped single line), and there are at least 3 items? We may want to make this configurable and off by default, but I'd like to enable such a thing for the code I work on.

Event Timeline

For arrays, we apparently already have a sniff, TrailingCommaSniff, written for T222042. It's not enabled by default and I had already forgotten it existed. It was also discussed in https://www.mediawiki.org/wiki/Manual_talk:Coding_conventions/PHP/Archive#Trailing_commas_in_multi-line_arrays, and the Squiz rule appeared to be problematic (and I'll totally believe that, given my experience with upstream sniffs for T340909 and T319352).

In general I think it is a good practice to have the same coding conventions for both php and js, so that devs do not need to remember 2 sets of conventions.

Right now the js linter enforces the opposite of this (not putting a comma at the end). I think I investigated why one time and determined that the idea was to make it easy to copy and paste things into json format, which requires no trailing commas.

Perhaps the js rule should be revisited if we are changing the php rule.

I said this gently at T396813#10911699, but here we are, so I'm afraid I'm going to have to be a bit more strident this time.

I think failing CI for a lack of a trailing comma is a waste of everyone's time. Same for a manual CR-1, or anything else which brings the issue to your attention more than a few seconds after you type the code. The problem of dirty diffs can be dealt with by putting up with them. Use a diff formatter that gives you intraline highlighting if you have trouble manually coping.

If I had no real work to do, maybe I would go around adding and subtracting trailing commas to make everything look neat. But I think there are bigger cosmetic issues with our codebase so it wouldn't be my first priority.

Incidentally, our eslint rules fail JS code when there is a trailing comma anywhere.

41:60  error  Unexpected trailing comma  comma-dangle

This rule should be removed from the config. When Parsoid was in JS, it failed CI if the trailing comma was absent. I think it is a waste of everyone's time either way.

A developer should get a lecture about the utility of trailing commas approximately once per lifetime. That is the level of attention this issue deserves.

I would like to +1 what @tstarling and others said above. Generally, the point of our code sniffer rule set is to make people's lives easier, not harder. Not allowing dangling commas made sense for a while as long as we had to support older JS engines that wouldn't understand them. But forcing everyone to add literally meaningless commas to their code? Why? To make the next developer's live easier? Come on. How often do you find yourself in this situation? It's literally a single key you have to press – in many cases not even that when your IDE automatically does it for you. To minimize diffs? Again, come on. It's a single comma right before a line you have to look at anyway.

Personally, I will certainly start adding dangling commas where I think they make sense. But I really don't want us to waste peoples time with a CI job that starts complaining about old code as if there would be anything wrong with it.

Obviously this would come with an automatic fixer, I don't want to spend time tweaking the code style either, that's what computers are for.

That said, if this is the mood that this topic brings up, then it's not worth doing. I thought it would be a welcome improvement, not a controversial discussion, but I was wrong. Sorry about the distraction.