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)

defect

Tracking

(firefox40 fixed)

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed
backlog tech-debt

People

(Reporter: standard8, Assigned: tomesh, Mentored)

References

Details

(Whiteboard: [lang=js])

Attachments

(1 file, 3 obsolete files)

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/
Flags: qe-verify-
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).
Hi, I would like to start working on this bug. Can you please assign me?
Assignee: nobody → tomesm
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-
Attachment #8601070 - Attachment is obsolete: true
Attachment #8601473 - Flags: review?(standard8)
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-
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?
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.
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.
Attachment #8601660 - Attachment is obsolete: true
Attachment #8601660 - Flags: review?(standard8)
Attachment #8602041 - Flags: review?(standard8)
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+
Rank: 38
Flags: firefox-backlog+
Priority: -- → P3
https://hg.mozilla.org/mozilla-central/rev/e4a44e449858
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.