Enable ESLint rule comma-dangle for all of mozilla-central

RESOLVED FIXED in Firefox 64

Status

()

enhancement
P1
normal
RESOLVED FIXED
10 months ago
8 months ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

Trunk
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 wontfix, firefox64 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Assignee

Description

10 months ago
As discussed on firefox-dev and dev.platform, we should require dangling commas for multi-line object/arrays in Javascript code for mozilla-central.

Why:

- Multiple components in toolkit/ and browser/ already have comma-dangle enabled.
- Having asked around in a few locations, I'm seeing support from the Firefox developer team and others in favour of enabling it.
- It helps make blame cleaner.
- It makes editing easier, and helps for a consistent style.
- Having automation available to cover the requirement reduces the need for review nits.

This bug is for the second half of the files, those not already enabled by bug 1486739.

I am hoping to land this bug on Friday 19 October (GMT timezone), ahead of the 64 merges.

Comment 1

10 months ago
This auto-rewriting won't touch anything in js/src/tests or js/src/jit-test, will it?  Absence of a trailing comma can't be presumed to be meaningless there, and much of the former is imported anyway.
Assignee

Comment 2

10 months ago
(In reply to Jeff Walden [:Waldo] from comment #1)
> This auto-rewriting won't touch anything in js/src/tests or js/src/jit-test,
> will it?  Absence of a trailing comma can't be presumed to be meaningless
> there, and much of the former is imported anyway.

They are ignored by default for ESLint, so they won't be touched: https://searchfox.org/mozilla-central/rev/55da592d85c2baf8d8818010c41d9738c97013d2/.eslintignore#284,292,294
I hope this change will also be applied to /testing?  Specifically
for testing/marionette we want to adopt as much of the repository-wide
lint rules as possible.
Assignee

Comment 4

10 months ago
(In reply to Andreas Tolfsen ﹝:ato﹞ from comment #3)
> I hope this change will also be applied to /testing?  Specifically
> for testing/marionette we want to adopt as much of the repository-wide
> lint rules as possible.

It will be applied for everywhere that ESLint is enabled (i.e. anywhere not ignored by .eslintignore). Note: comma-dangle is already enabled for testing/marionette, so all that will happen there is the sub-directory specific configuration will be removed in preference to the main one (tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js).
Assignee

Updated

9 months ago
Depends on: 1493967
Priority: -- → P1
Attachment #9016296 - Attachment is obsolete: true
Assignee

Comment 14

9 months ago
Note: Aiming for landing next Friday (19th Oct)

Comment 15

8 months ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/edc4ae8f78e2
Enable ESLint rule comma-dangle for all of mozilla-central (automatic fixes). r=mossop
https://hg.mozilla.org/integration/autoland/rev/988056e6d054
Enable ESLint rule comma-dangle for all of mozilla-central (manual fixes). r=mossop
https://hg.mozilla.org/integration/autoland/rev/1ac84aa579d6
Enable ESLint rule comma-dangle for all of mozilla-central (setup ESLint). r=mossop

Comment 16

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/edc4ae8f78e2
https://hg.mozilla.org/mozilla-central/rev/988056e6d054
https://hg.mozilla.org/mozilla-central/rev/1ac84aa579d6
Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.