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)

3 Branch
enhancement
Not set
normal

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
you can assign this to me i will work on this.
Assignee: nobody → rajesh.kathiriya507
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 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 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)
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 :-)
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Product: Testing → Firefox Build System
Keywords: good-first-bug
Version: Version 3 → 3 Branch
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: