Closed
Bug 1160487
Opened 9 years ago
Closed 9 years ago
Enable eslint rules for Loop: semi-colon related
Categories
(Hello (Loop) :: Client, defect, P3)
Hello (Loop)
Client
Tracking
(firefox40 fixed)
People
(Reporter: standard8, Assigned: tomesh, Mentored)
References
Details
(Whiteboard: [lang=js])
Attachments
(1 file, 3 obsolete files)
56.01 KB,
patch
|
standard8
:
review+
|
Details | Diff | Splinter Review |
Enable some more eslint rules for Loop. Rules to enable in this bug: * no-extra-semi - disallows extra semi-colons * semi - ensures semi-colons are in the right places * semi-spacing - ensures there's a space after semi-colons (or a newline) To enable the rule, remove the line from browser/components/loop/.eslintrc and save it. Then you can run eslint in the browser/components/loop directory with: eslint --ext .js --ext .jsm --ext .jsx . The errors listed in the output are to be fixed. If you need more help setting up eslint, see https://wiki.mozilla.org/Loop/Development#Additional_Requirements Information about the eslint rules can be found here: http://eslint.org/docs/rules/
Reporter | ||
Updated•9 years ago
|
Flags: qe-verify-
Reporter | ||
Comment 1•9 years ago
|
||
Note: if there's errors in a .js file that has an associated .jsx file, then fix the jsx file and run the react tools to generate the .js file (https://wiki.mozilla.org/Loop/Development#Developing).
Assignee | ||
Comment 2•9 years ago
|
||
Hi, I would like to start working on this bug. Can you please assign me?
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8601070 -
Flags: review?(standard8)
Reporter | ||
Updated•9 years ago
|
Assignee: nobody → tomesm
Reporter | ||
Comment 4•9 years ago
|
||
Comment on attachment 8601070 [details] [diff] [review] Eslint semicolon rules enabled. Affected files corrected accordingly. Review of attachment 8601070 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch, this is looking good. There's a few additional changes that I think will make it even better and reading the code a bit easier. ::: browser/components/loop/content/shared/js/utils.js @@ +17,5 @@ > mozL10n = { get: function() { > throw new Error("mozL10n.get not availabled from chrome!"); > }}; > } else { > + mozL10n = document.mozL10n || navigator.mozL10n; } I think the } got moved onto the wrong line by mistake here. ::: browser/components/loop/modules/LoopCalls.jsm @@ +53,5 @@ > */ > connect: function(onSuccess, onError) { > this._onSuccess = onSuccess; > this._onError = onError || > + (reason => {MozLoopService.log.warn("LoopCalls::callProgessSocket - ", reason); }); I think for this line we should split it: this._onError = onError || (reason => { MozLoopService.log.warn("LoopCalls::callProgessSocket - ", reason); }); as it looks a bit better. @@ +236,5 @@ > */ > > _getCalls: function(sessionType, version) { > return MozLoopService.hawkRequest(sessionType, "/calls?version=" + version, "GET").then( > + response => {this._processCalls(response, sessionType); } For this, and the rest of the instances of {...;} where you're adding a space, could you add a space at the start of the section as well please? e.g. response => { this._processCalls(response, sessionType); } I know the eslint pattern doesn't call for it at the moment, but I think it'll look better like that overall. ::: browser/components/loop/test/standalone/standalone_client_test.js @@ +93,5 @@ > sinon.assert.calledOnce(console.error); > sinon.assert.calledWithExactly(console.error, "Server error", > "HTTP 404 Not Found", serverResponse); > }); > + }); nit: there's a trailing space on this line that we don't want added.
Attachment #8601070 -
Flags: review?(standard8) → review-
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8601070 -
Attachment is obsolete: true
Attachment #8601473 -
Flags: review?(standard8)
Reporter | ||
Comment 6•9 years ago
|
||
Comment on attachment 8601473 [details] [diff] [review] rev2 - Trailings removed. Code updated to looks better. Review of attachment 8601473 [details] [diff] [review]: ----------------------------------------------------------------- That's an improvement, but I think you missed a couple of bits. ::: browser/components/loop/content/shared/js/utils.js @@ +20,2 @@ > } else { > + mozL10n = document.mozL10n || navigator.mozL10n; } I think you missed putting this } on the new-line. ::: browser/components/loop/modules/LoopCalls.jsm @@ +53,5 @@ > */ > connect: function(onSuccess, onError) { > this._onSuccess = onSuccess; > this._onError = onError || > + (reason => {MozLoopService.log.warn("LoopCalls::callProgessSocket - ", reason); Please put the (reason => { part on the previous line, just have the bit starting MozLoopService here. @@ +397,5 @@ > callProgress._websocket = this.mocks.webSocket; > } > // This instance of CallProgressSocket should stay alive until the underlying > // websocket is closed since it is passed to the websocket as the nsIWebSocketListener. > + callProgress.connect(() => {callProgress.sendBusy(); }); I think you missed fixing the space after { for this one, and the ones in MozLoopPushHandler.jsm/MozLoopService.jsm/otSdkDriver_test.js
Attachment #8601473 -
Flags: review?(standard8) → review-
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8601473 -
Attachment is obsolete: true
Attachment #8601660 -
Flags: review?(standard8)
Reporter | ||
Comment 8•9 years ago
|
||
Comment on attachment 8601660 [details] [diff] [review] rev3 - Code readability improvement (added spaces when necessary). Overlooked mistakes corrected. Review of attachment 8601660 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/loop/modules/MozLoopService.jsm @@ +8,4 @@ > > +// invalid auth token as per > +// https://github.com/mozilla-services/loop-server/blob/45787d34108e2f0d87d74d4ddf4ff0dbab23501c/loop/errno.json#l6 > +const invalid_auth_token = 110; Something seems to have taken all the capitals from this file? Did you attach the wrong version?
Assignee | ||
Comment 9•9 years ago
|
||
No :( Dunno what've happened. I've just run hg qref as usual after updating files... is there any mercurial command to revert patch changes up to "previous" version and try it again? I can see now there are so many "invalid" errors in the patch file :( Dunno what to do know.
Reporter | ||
Comment 10•9 years ago
|
||
Unfortunately hg qref clobbers the record of your previous changes. You could revert the whole file to its existing state with something like: hg diff -r "p1(tip)" browser/components/loop/modules/MozLoopService.jsm | patch -p1 -R and then re-apply the changes to the MozLoopService.jsm file.
Assignee | ||
Comment 11•9 years ago
|
||
Attachment #8601660 -
Attachment is obsolete: true
Attachment #8601660 -
Flags: review?(standard8)
Attachment #8602041 -
Flags: review?(standard8)
Reporter | ||
Comment 12•9 years ago
|
||
Comment on attachment 8602041 [details] [diff] [review] rev4 - Code readability improvement (added spaces when necessary). Overlooked mistakes corrected. Review of attachment 8602041 [details] [diff] [review]: ----------------------------------------------------------------- Looks great. There's a couple of minor things that weren't quite right, but this is close enough that I can just address those on check-in. I'll land this in a little bit. ere's the additional changes I'm making (I don't normally like pasting diffs directly, but this one is smallish): diff --git a/browser/components/loop/content/shared/js/utils.js b/browser/components/loop/content/shared/js/utils.js index a905b2d..b108354 100644 --- a/browser/components/loop/content/shared/js/utils.js +++ b/browser/components/loop/content/shared/js/utils.js @@ -15,8 +15,8 @@ var inChrome = typeof Components != "undefined" && "utils" in Components; if (inChrome) { this.EXPORTED_SYMBOLS = ["utils"]; mozL10n = { get: function() { - throw new Error("mozL10n.get not availabled from chrome!"); } - }; + throw new Error("mozL10n.get not availabled from chrome!"); + }}; } else { mozL10n = document.mozL10n || navigator.mozL10n; } diff --git a/browser/components/loop/modules/LoopCalls.jsm b/browser/components/loop/modules/LoopCalls.jsm index 4176830..6a364e4 100644 --- a/browser/components/loop/modules/LoopCalls.jsm +++ b/browser/components/loop/modules/LoopCalls.jsm @@ -53,7 +53,8 @@ CallProgressSocket.prototype = { */ connect: function(onSuccess, onError) { this._onSuccess = onSuccess; - this._onError = onError || (reason => { MozLoopService.log.warn("LoopCalls::callProgessSocket - ", reason); + this._onError = onError || (reason => { + MozLoopService.log.warn("LoopCalls::callProgessSocket - ", reason); }); if (!onSuccess) {
Attachment #8602041 -
Flags: review?(standard8) → review+
Updated•9 years ago
|
Rank: 38
Flags: firefox-backlog+
Priority: -- → P3
Comment 14•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e4a44e449858
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in
before you can comment on or make changes to this bug.
Description
•