Page MenuHomePhabricator

switch eslint to require dangling comma with "always-multiline"
Open, Needs TriagePublic

Description

Having a dangling comma in multiline javascript objects and arrays results in cleaner commit diffs if the last line is removed or an item is added to the end. For some reason these trailing commas are forbidden in mediawiki's eslint config.

This suggests to change for our repositories the linting configuration from "comma-dangle": ["error", "never"] to "comma-dangle": ["error", "always-multiline"]. The respective places in code can be fixed automatically by eslint.

https://eslint.org/docs/rules/comma-dangle

acceptance criteria
eslint is changed and code adjusted for the following repos:

  • Wikibase
  • WikibaseLexeme
  • WikibaseQualityConstraints
  • EntitySchema
  • FIXME -- do we maintain other repos with javascript/eslint?

Event Timeline

Michael created this task.May 7 2019, 11:01 AM
Restricted Application added a project: Wikidata. · View Herald TranscriptMay 7 2019, 11:01 AM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript

The coding standard you link is not "MediaWiki's", it's Wikimedia's, and is meant to apply to all production code. I disagree with your argument (the developer community has had this conversation before and decided that easier portability to JSON is more important than cleaner diffs), but if you want to start a new conversation about changing it I'd be happy to facilitate.

The coding standard you link is not "MediaWiki's", it's Wikimedia's, and is meant to apply to all production code. I disagree with your argument (the developer community has had this conversation before and decided that easier portability to JSON is more important than cleaner diffs), but if you want to start a new conversation about changing it I'd be happy to facilitate.

I would like to read up on the existing discussion first, because the provided argument seems implausible to me and I would like to understand better first, before starting the discussion anew.

However, I cannot find the discussion in any of the subpages from the link you provided. See https://w.wiki/3ix
Also, the tasks of Front-end-Standards-Group don't seem to contain this discussion: https://phabricator.wikimedia.org/search/query/go0g6aHL_QY8/#R
And this rule was already there in the first commit of the GitHub repo.

Do you happen to remember where that conversation might be archieved?

The coding standard you link is not "MediaWiki's", it's Wikimedia's, and is meant to apply to all production code. I disagree with your argument (the developer community has had this conversation before and decided that easier portability to JSON is more important than cleaner diffs), but if you want to start a new conversation about changing it I'd be happy to facilitate.

I would like to read up on the existing discussion first, because the provided argument seems implausible to me and I would like to understand better first, before starting the discussion anew.
However, I cannot find the discussion in any of the subpages from the link you provided. See https://w.wiki/3ix
Also, the tasks of Front-end-Standards-Group don't seem to contain this discussion: https://phabricator.wikimedia.org/search/query/go0g6aHL_QY8/#R

I don't know where the discussion would have been documented, if anywhere. I vaguely recall some discussion about it as an existing rule in ~2012/2013 in terms of consistency with JSON.

And this rule was already there in the first commit of the GitHub repo.

Yes, it was an existing rule when we switched from jscs to eslint and so was imported from those rules.

In our jscs ruleset it was originally implemented back in 2014 as a codification of what the coding conventions document instructed then.

Before we used jscs, it was previously enforced via jshint ("trailing": true). The first version of the JS-specific coding conventions split out by @Krinkle in December 2011 said:

When using an object literal, don't include a trailing comma. var obj = { a: 1, b: 2, }; will fail in some versions of IE while var obj = { a: 1, b: 2 }; will work universally.

It was originally added to our coding standards in this edit by @DanielFriesen.

Obviously the need to not crash IE4 is now gone, but I don't know if any proper conversation has been had about changing this convention. The casual discussion I remember about JSON-style certainly wasn't a full process.

Hope this archæology helps!

Thank you a lot! I appreciate this digging into ancient history :)

I now understand better how this rule came to be, thank you 🙏

These missing commas and thus suboptimal diffs are only a very minor annoyance at most and I'm not sure they are worth the time and effort of a full process, if there are strong feelings in favor of the current policy.

Thank you for providing this context.

Object literal trailing comma support was only added to IE in version 9. IE8 support was only dropped a few years ago. We also need to support IE8 in the resource loader init file, which is currently enforceable just by setting the environment to ES3, so this would add another exception.

Most non-command line differs do a partial line diff so this isn't much of an issue for me. And as pointed out PHP has them but JSON doesn't so there will never be consistency either way.