Enable the ESLint comma-style rule across mozilla-central (commas at end of line)

RESOLVED FIXED in Firefox 56

Status

RESOLVED FIXED
a year ago
8 months ago

People

(Reporter: standard8, Assigned: rajk, Mentored)

Tracking

({good-first-bug})

Version 3
mozilla56
good-first-bug

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [lang=js], URL)

Attachments

(1 attachment, 1 obsolete attachment)

Across the tree we have approximately 70000 instances of commas at the end of lines, and approx 500 instances at the start of the line.

Most of the 500 instances are in places. I confirmed with Marco on irc that he's happy with us changing those to match the rest.

I think we should therefore make this a default rule, to help implement a consistent style across the tree.

I'm happy to mentor this bug. There's background on our eslint setups here:

https://developer.mozilla.org/docs/ESLint

Here's some approximate steps:

- Uncomment the comma-style line in recommended.js.
- Remove the comma-style related lines in the existing .eslintrc.js configs (apart from devtools/):

https://dxr.mozilla.org/mozilla-central/search?q=comma-style&redirect=false

- Run eslint for fixing:

./mach eslint --fix

This should fix most/all of the instances.

- Fix any remaining instances
- Inspect the diff to make sure that the indentation still looks alright
- Create a commit and push it to mozreview:

http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview.html
(Assignee)

Comment 1

a year ago
you can assign this to me i will work on this.
Assignee: nobody → rajesh.kathiriya507
Comment hidden (mozreview-request)
(Reporter)

Comment 3

a year ago
mozreview-review
Comment on attachment 8878102 [details]
Bug 1370225 - Enabled the ESLint comma-style rule across mozilla-central.

https://reviewboard.mozilla.org/r/149506/#review154412

Looks good so far - unfortuantely --fix isn't quite getting the indentation levels right, please can you go through and check those. I've highlighted some of the cases that need correcting.

::: browser/base/content/browser-places.js:576
(Diff revision 1)
>     */
>    async bookmarkLink(aParentId, aURL, aTitle, aDescription = "") {
>      let node = await PlacesUIUtils.fetchNodeLike({ url: aURL });
>      if (node) {
> -      PlacesUIUtils.showBookmarkDialog({ action: "edit"
> -                                       , node
> +      PlacesUIUtils.showBookmarkDialog({ action: "edit",
> +                                        node

nit: please check the sub-indentation of the changes in this file - for example, this line needs one more space of intendation.

::: browser/components/places/content/controller.js:296
(Diff revision 1)
>      case "placesCmd_createBookmark":
>        let node = this._view.selectedNode;
> -      PlacesUIUtils.showBookmarkDialog({ action: "add"
> -                                       , type: "bookmark"
> -                                       , hiddenRows: [ "description"
> -                                                     , "keyword"
> +      PlacesUIUtils.showBookmarkDialog({ action: "add",
> +                                         type: "bookmark",
> +                                         hiddenRows: [ "description",
> +                                                      "keyword",

nit: these items need to be one-space further indented

::: browser/components/places/tests/unit/test_clearHistory_shutdown.js:22
(Diff revision 1)
>  
>  const TOPIC_CONNECTION_CLOSED = "places-connection-closed";
>  
>  var EXPECTED_NOTIFICATIONS = [
> -  "places-shutdown"
> -, "places-will-close-connection"
> +  "places-shutdown",
> + "places-will-close-connection",

nit: indentation

::: browser/modules/ContentClick.jsm:56
(Diff revision 1)
> -                                       , type: "bookmark"
> -                                       , uri: Services.io.newURI(json.href)
> -                                       , title: json.title
> -                                       , loadBookmarkInSidebar: true
> -                                       , hiddenRows: [ "description"
> -                                                     , "location"
> +                                         type: "bookmark",
> +                                         uri: Services.io.newURI(json.href),
> +                                         title: json.title,
> +                                         loadBookmarkInSidebar: true,
> +                                         hiddenRows: [ "description",
> +                                                      "location",

nit: indentation

::: toolkit/components/places/Bookmarks.jsm:188
(Diff revision 1)
>      }
>      let insertInfo = validateBookmarkObject(info,
> -      { type: { defaultValue: this.TYPE_BOOKMARK }
> -      , index: { defaultValue: this.DEFAULT_INDEX }
> -      , url: { requiredIf: b => b.type == this.TYPE_BOOKMARK
> -             , validIf: b => b.type == this.TYPE_BOOKMARK }
> +      { type: { defaultValue: this.TYPE_BOOKMARK },
> +        index: { defaultValue: this.DEFAULT_INDEX },
> +        url: { requiredIf: b => b.type == this.TYPE_BOOKMARK,
> +              validIf: b => b.type == this.TYPE_BOOKMARK },

nit: indentation

::: toolkit/components/places/Bookmarks.jsm:191
(Diff revision 1)
> -      , index: { defaultValue: this.DEFAULT_INDEX }
> -      , url: { requiredIf: b => b.type == this.TYPE_BOOKMARK
> -             , validIf: b => b.type == this.TYPE_BOOKMARK }
> -      , parentGuid: { required: true }
> -      , title: { validIf: b => [ this.TYPE_BOOKMARK
> -                               , this.TYPE_FOLDER ].includes(b.type) }
> +        index: { defaultValue: this.DEFAULT_INDEX },
> +        url: { requiredIf: b => b.type == this.TYPE_BOOKMARK,
> +              validIf: b => b.type == this.TYPE_BOOKMARK },
> +        parentGuid: { required: true },
> +        title: { validIf: b => [ this.TYPE_BOOKMARK,
> +                                this.TYPE_FOLDER ].includes(b.type) },

nit: indentation

::: toolkit/components/places/Bookmarks.jsm:347
(Diff revision 1)
>          // dateAdded may be imposed by the caller.
>          let time = (info && info.dateAdded) || fallbackLastAdded;
>          let insertInfo = validateBookmarkObject(info, {
> -          type: { defaultValue: TYPE_BOOKMARK }
> -          , url: { requiredIf: b => b.type == TYPE_BOOKMARK
> -                 , validIf: b => b.type == TYPE_BOOKMARK }
> +          type: { defaultValue: TYPE_BOOKMARK },
> +          url: { requiredIf: b => b.type == TYPE_BOOKMARK,
> +                  validIf: b => b.type == TYPE_BOOKMARK },

nit: check indentation of sub-items generally in this file.

::: toolkit/components/places/PlacesSyncUtils.jsm:1357
(Diff revision 1)
>  function validateNewBookmark(info) {
>    let insertInfo = validateSyncBookmarkObject(info,
> -    { kind: { required: true }
> -    , syncId: { required: true }
> -    , url: { requiredIf: b => [ BookmarkSyncUtils.KINDS.BOOKMARK
> -                              , BookmarkSyncUtils.KINDS.QUERY ].includes(b.kind)
> +    { kind: { required: true },
> +      syncId: { required: true },
> +      url: { requiredIf: b => [ BookmarkSyncUtils.KINDS.BOOKMARK,
> +                               BookmarkSyncUtils.KINDS.QUERY ].includes(b.kind),

nit: check indentation in this section
Attachment #8878102 - Flags: review?(standard8)
Comment hidden (mozreview-request)
(Reporter)

Comment 5

a year ago
mozreview-review
Comment on attachment 8878102 [details]
Bug 1370225 - Enabled the ESLint comma-style rule across mozilla-central.

https://reviewboard.mozilla.org/r/149506/#review154802

On the right track, there's a few accidents that seem to have happened though.

::: browser/base/content/browser-places.js:585
(Diff revision 2)
>  
>      let ip = new InsertionPoint(aParentId,
>                                  PlacesUtils.bookmarks.DEFAULT_INDEX,
>                                  Components.interfaces.nsITreeView.DROP_ON);
> -    PlacesUIUtils.showBookmarkDialog({ action: "add"
> -                                     , type: "bookmark"
> +    PlacesUIUtils.showBookmarkDialog({ action: "add",
> +																																							type: "bookmark",

Something seems to have gone wrong here and give this a really big indent.

There's a few locations like this, if you check through the mozreview page here: 

https://reviewboard.mozilla.org/r/149506

you should see them quite easily.

::: toolkit/components/places/Bookmarks.jsm:353
(Diff revision 2)
> -          , parentGuid: { required: true }
> -          , title: { validIf: b => [ TYPE_BOOKMARK
> -                                   , TYPE_FOLDER ].includes(b.type) }
> -          , dateAdded: { defaultValue: time
> -                       , validIf: b => !b.lastModified ||
> -                                        b.dateAdded <= b.lastModified }
> +          parentGuid: { required: true },
> +          title: { validIf: b => [ TYPE_BOOKMARK,
> +                                   TYPE_FOLDER ].includes(b.type) },
> +          dateAdded: { defaultValue: time,
> +                       validIf: b => !b.lastModified ||
> +                                        b.dateAdded <= b.lastModified },

nit: still a couple of sub-indentations wrong in this file.

::: toolkit/components/places/PlacesSyncUtils.jsm:1368
(Diff revision 2)
> -                             , BookmarkSyncUtils.KINDS.FOLDER
> -                             , BookmarkSyncUtils.KINDS.LIVEMARK ].includes(b.kind) }
> -    , query: { validIf: b => b.kind == BookmarkSyncUtils.KINDS.QUERY }
> -    , folder: { validIf: b => b.kind == BookmarkSyncUtils.KINDS.QUERY }
> -    , tags: { validIf: b => [ BookmarkSyncUtils.KINDS.BOOKMARK
> -                            , BookmarkSyncUtils.KINDS.QUERY ].includes(b.kind) }
> +																															BookmarkSyncUtils.KINDS.FOLDER,
> +																															BookmarkSyncUtils.KINDS.LIVEMARK ].includes(b.kind) },
> +      query: { validIf: b => b.kind == BookmarkSyncUtils.KINDS.QUERY },
> +      folder: { validIf: b => b.kind == BookmarkSyncUtils.KINDS.QUERY },
> +      tags: { validIf: b => [ BookmarkSyncUtils.KINDS.BOOKMARK,
> +                             	BookmarkSyncUtils.KINDS.QUERY ].includes(b.kind) },

nit: some tabs (rather than spaces) have creapt into this file.
Attachment #8878102 - Flags: review?(standard8)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 9

a year ago
mozreview-review
Comment on attachment 8879037 [details]
Bug 1370225 - Enabled the ESLint comma-style rule across mozilla-central.

https://reviewboard.mozilla.org/r/150352/#review154962

Hi Rajesh, there were still a couple of issues with tabs instead of spaces, so I fixed those up for you so we can get this landed. To avoid this in the future, I'd suggest looking at the output of git diff, or the output in mozreview - it is generally pretty obvious when it happens. At some stage we'll also think about deploying the indent rule more widely, and that would inform writers as well.
Attachment #8879037 - Flags: review?(standard8) → review+
Attachment #8878102 - Attachment is obsolete: true
Attachment #8878102 - Flags: review?(standard8)

Comment 10

a year ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f43794aefcc1
Enabled the ESLint comma-style rule across mozilla-central. r=standard8
Thank you for you work on this Rajesh, nice to get it landed :-)

Comment 12

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f43794aefcc1
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56

Updated

8 months ago
Product: Testing → Firefox Build System
You need to log in before you can comment on or make changes to this bug.