Closed
Bug 1370225
Opened 8 years ago
Closed 8 years ago
Enable the ESLint comma-style rule across mozilla-central (commas at end of line)
Categories
(Developer Infrastructure :: Lint and Formatting, enhancement)
Tracking
(firefox56 fixed)
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: standard8, Assigned: rajk, Mentored)
References
()
Details
(Whiteboard: [lang=js])
Attachments
(1 file, 1 obsolete file)
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•8 years ago
|
||
you can assign this to me i will work on this.
Reporter | ||
Updated•8 years ago
|
Assignee: nobody → rajesh.kathiriya507
Comment hidden (mozreview-request) |
Reporter | ||
Comment 3•8 years 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•8 years 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•8 years 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+
Reporter | ||
Updated•8 years ago
|
Attachment #8878102 -
Attachment is obsolete: true
Attachment #8878102 -
Flags: review?(standard8)
Comment 10•8 years 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
Reporter | ||
Comment 11•8 years ago
|
||
Thank you for you work on this Rajesh, nice to get it landed :-)
Comment 12•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Updated•7 years ago
|
Product: Testing → Firefox Build System
Updated•6 years ago
|
Keywords: good-first-bug
Version: Version 3 → 3 Branch
Updated•2 years ago
|
Product: Firefox Build System → Developer Infrastructure
You need to log in
before you can comment on or make changes to this bug.
Description
•