Closed
Bug 1359540
Opened 7 years ago
Closed 7 years ago
Enable the ESLint recommended rules for services/
Categories
(Firefox :: Sync, defect)
Firefox
Sync
Tracking
()
RESOLVED
FIXED
Firefox 55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: standard8, Assigned: dbugs, Mentored)
References
Details
Attachments
(1 file)
I'm preparing to get the general ESLint rules we have in the eslint-plugin-mozilla recommended configuration spread around the tree more widely. At the moment, services/ doesn't inherit rules from the mozilla/recommended configuration, and so just gets a basic set. The aim of this bug is to get services/ passing mozilla/recommended and for it to be enabled by default.
Comment hidden (mozreview-request) |
Comment 2•7 years ago
|
||
mozreview-review |
Comment on attachment 8862501 [details] Bug 1359540 - Enable the Mozilla ESlint recommended rules for services/. https://reviewboard.mozilla.org/r/134404/#review137412 Thanks for the patch and for your work improving the general quality of Firefox, but there are a number of issues here that need to be addressed. I'm happy to take another look when you update the patch! ::: services/crypto/modules/utils.js:514 (Diff revision 1) > // trailing "==" padding. > let hash_b64 = btoa(hash); > artifacts.hash = hash_b64; > } > > - let requestString = ("hawk.1.header" + "\n" + > + let requestString = ("hawk.1.headerFxAccountsManager.jsm\n" + The addition of "FxAccountsManager.jsm" here looks wrong ::: services/fxaccounts/FxAccountsManager.jsm:468 (Diff revision 1) > } > > let client = this._getFxAccountsClient(); > return client.accountExists(aEmail).then( > result => { > - log.debug("Account " + result ? "" : "does not" + " exists"); > + log.debug("Account " + result ? "" : "does not exists"); Before this change, the resulting string would be either "Account exists" or "Account does not exists" (even though that last one is bad grammar :) However, with your change the string with be either "Account " or "Account does not exists". I'm not sure why eslint is complaining about this line - it might be that you just need to add parens (ie, brackets) - ie, |"Account " + (result ? "" : "does not" + " exists")|, but I'm not sure. ::: services/sync/modules/bookmark_repair.js:384 (Diff revision 1) > break; // our caller will take the abort action. > > case STATE.FINISHED: > break; > > - case NOT_REPAIRING: > + case STATE.NOT_REPAIRING: this change looks like it is fixing a real bug - thanks! :) ::: services/sync/modules/telemetry.js:142 (Diff revision 1) > let took = timeDeltaFrom(this.startTime); > if (took > 0) { > this.took = took; > } > if (error) { > - this.failureReason = SyncTelemetry.transformError(error); > + this.failureReason = this.SyncTelemetry.transformError(error); This change is wrong - |this| here will be an EngineRecord, not the module scope where SyncTelemetry is defined. I pulled this code to try and see what eslint was complaining about here, but when I couldn't see any eslint errors without the additional |this|, so I'm not sure why this change is necessary (but if it is, a solution might be to change the last line of the file from: this.SyncTelemetry = new SyncTelemetryImpl(ENGINES); to something like: let SyncTelemetry = this.SyncTelemetry = new SyncTelemetryImpl(ENGINES); (but even that seems strange and I'd have thought unnecessary) ::: services/sync/tests/unit/head_helpers.js:488 (Diff revision 1) > Utils.getDefaultDeviceName = function() { > return "Test device name"; > }; > > function registerRotaryEngine() { > + let RotaryEngine = This needs to read |let {RotaryEngine} =| (or maybe |let { RotaryEngine } =|, depending on what style we are heading towards - :Standard8 probably knows)
Attachment #8862501 -
Flags: review?(markh)
Comment hidden (mozreview-request) |
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8862501 [details] Bug 1359540 - Enable the Mozilla ESlint recommended rules for services/. https://reviewboard.mozilla.org/r/134404/#review138192 Awesome, thanks!
Attachment #8862501 -
Flags: review?(markh) → review+
Pushed by mbanner@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e6177d883f37 Enable the Mozilla ESlint recommended rules for services/. r=markh
Comment 6•7 years ago
|
||
backed out for eslint failures like https://treeherder.mozilla.org/logviewer.html#?job_id=95836836&repo=autoland
Flags: needinfo?(dbugs)
Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7f06e5ac97e6 Backed out changeset e6177d883f37 for eslint failures
Reporter | ||
Comment 8•7 years ago
|
||
It looks like these were failures are due to bug 1350030 that landed overnight, so the patch will need updating.
Comment hidden (mozreview-request) |
Reporter | ||
Comment 10•7 years ago
|
||
The additional changes due to bug 1350030 are minor, and I've checked them in the updated patch and rs=me for those extra bits. I'll get this re-landed.
Flags: needinfo?(dbugs)
Comment 11•7 years ago
|
||
Pushed by mbanner@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/20e33f2530a1 Enable the Mozilla ESlint recommended rules for services/. r=markh
Comment 12•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/20e33f2530a1
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in
before you can comment on or make changes to this bug.
Description
•